Basically brought back what we had from before (with some minor tweaks): https://github.com/CommE2E/comm/blob/3a79d65da519971ea4f3000699f67fea75a9754c/landing/header.css
Details
Details
Here's how it looks:
How it looks on iPhone 13 mini (layout):
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I'm not sure how I feel about the white circles... can you share a video of what it would look like without those?
Comment Actions
I'm not sure how I feel about the white circles... can you share a video of what it would look like without those?
Sure, can try something
Mainly did the white circles for the GitHub button since their brand color is dark and blends into the background without the contrast added by the white circle
Will try a gray to white on hover, white to gray on hover, and a white/gray to brand color on hover and add to this diff
Comment Actions
I didn't do a full review. But, these were a couple of things I thought were worth looking at before landing.
| landing/header.css | ||
|---|---|---|
| 57 ↗ | (On Diff #10309) | Can we use a rem unit? |
| 58 ↗ | (On Diff #10309) | why margin-inline? Can we just use padding? https://mxstbr.com/thoughts/margin/ Margin has always borked layout for me and I try to stay away from it was much as possible. |
| landing/header.react.js | ||
| 44 ↗ | (On Diff #10309) | Can we just add the class directly to the SVG element and style it from there and skip using another div all together? |
| landing/header.css | ||
|---|---|---|
| 57 ↗ | (On Diff #10309) | I'd rather stay away from rem units until the issues in global.css are resolved |
| 58 ↗ | (On Diff #10309) | Because I want the space in between the icons to be "dead" and not stretch the clickable region of the icons
I get that the idea that, for example, the SubscriptionForm component shouldn't have margin "around it" so it can be used in different contexts without weird side effects... but, in this case we're applying the margin to the child elements of a container which seems totally fine to me? I feel like both margin and padding have their place, and in this context margin makes more sense to me. |
| landing/header.react.js | ||
| 44 ↗ | (On Diff #10309) |
The div has its own styles outside of the SVG. For example, it has fixed dimensions (44px x 44px) for the circle that appears on hover (the icons have varying dimensions, so without fixing the dimensions of the divs we'd get different oval shapes for each icon on hover). |
Comment Actions
(Note this diff is straight from what was accepted and landed before, so my instinct is that it's okay to go with for now since it's more of a revert than introduction of something new)
Comment Actions
Option 4 won't load and I can't tell the difference between 1 and 3. I assume having a color hover state (without the circle) doesn't look good?
(Note this diff is straight from what was accepted and landed before, so my instinct is that it's okay to go with for now since it's more of a revert than introduction of something new)
Makes sense. I think I prefer any of the options above to the original solution... probably 1/3 > 2, but I don't feel strongly