-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New design for Staking cards #13115 #14047
New design for Staking cards #13115 #14047
Conversation
… box size to StakingBadge, change button variant and add target and rel attributes to social links
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@nloureiro what do you think? ping for visibility |
@konopkja Meanwhile, I can provide the fix for this; I definitely missed that on the dark mode. But the images you provided in the layout are way better than using the black svgs as I am, if someone have some info on how to get those to run, would be awesome. |
I'm not sure what we have on the repo, but if we can´t change the colors on the theme, we can leave the original logos, too. |
@rafagomes great work. Thank you! I did a review, and there were small design adjustments. If you want to check this on Figma, here is the link > https://www.figma.com/design/wdBEEuqbT2t94jp92DTITy/New-Staking-cards?node-id=76-645&t=9dXEjaMNpWJaM6oh-1 Let me know if you need any further help. |
…into feature/13115-staking-card-design
@nloureiro @konopkja @wackerow Hey guys, PR opened with fixes requested by @nloureiro. Cheers! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rafagomes! Thanks for jumping in and working on this
I'm curious if you're familiar with Tailwind, as we've been steadily updating our repo from Chakra-UI to Tailwind for styling and shadcn for our components. It would be great to update this using Tailwind and removing the Chakra-UI dependencies from this component.
On that note, we should not be touching src/theme.ts
as this entire file is on its way out the door, being deprecated over time. Ideally we work with the existing colors inside tailwind.config.ts
which are looking at semantic-tokens.css
CSS variables.
Separately, one thing I'm noting so far is that the new design uses full color icons, which we'll need to gather from the different products, since the current icons being used are monochromatic.
In my opinion, SVG versions would be nice if we can obtain those, but a PNG would also work. We'll probably have some cases where we need different assets depending on color mode.
Let us know your comfort level with Tailwind and we can take things from there, thanks!
@nloureiro Curious if you see the color icons as a blocker, as we could potentially just use the existing ones in the meantime. I think it's probably worth just gathering them now though |
I'll do it. Should I refactor all components that are dependent on Tailwind? Do you know what we are using for icons on Tailwind? About the icons, I already discussed with @nloureiro maybe an issue for that would be great. I can pick it up next. |
Just pushed a few more adjustments which I think put this in a mergeable place... will let build first. Some notable TODO's that are not part of this:
We have some fee data, but we need to think about how we want to display this given not all fee structures are the same. Some are fixed monthly rates in different currencies, some are percentage based, etc. Also wouldn't hurt to make sure the fee data is up-to-date before going forward with that. |
use `[&svg]:` to style SVG from parent as-needed
@wackerow @nloureiro Fixed the logos, completing the work that @wackerow did. Now Figment and RockX are working on both light and dark. Also reduced the size of Socials for 8. I think now we are good to go <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafagomes Left some thoughts! I think the Figment icon is fine, but would prefer we update the RockX one... let me know if you'd like to handle this, or if you'd prefer me to jump on it.
@@ -3,11 +3,11 @@ import { createIconBase } from "../icon-base" | |||
export const FigmentGlyphIcon = createIconBase({ | |||
displayName: "FigmentGlyphIcon", | |||
viewBox: "0 0 32 32", | |||
className: "h-auto w-[32px]", | |||
className: "size-[1em] bg-black", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my other note, I'm not a huge fan of styling these icons this way... but in this specific case, I don't think I'll push back much... This icon is a little strange to begin with compared to the others, and I think how you have it works just fine.
Just noting that this approach is laying a "black" background, then overlaying that entire background with a rect
using currentColor
at an opacity of 20%... then placing a "white" path on top of it. Little messy... but it's working for now.
This is all keeping in mind that we'll soon migrate these to colored branch icons as well, which will upend these.
@@ -5,7 +5,7 @@ import { createIconBase } from "../icon-base" | |||
export const RockXGlyphIcon = createIconBase({ | |||
displayName: "RockXGlyphIcon", | |||
viewBox: "0 0 32 32", | |||
className: "h-auto w-[32px]", | |||
className: "size-[1em] bg-black p-2 dark:bg-background-highlight", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rafagomes! I see where you're going here, but wouldn't recommend this approach here tbh. Ideally we avoid declaring specific colors here as much as possible, unless we really want to force them. In this case, we still aren't applying the proper brand colors, so we want this to be responsive to grayscale depending on light or dark mode. ie, we want it to invert.
Playing locally, I updated this icon to the following:
import { commonIconDefaultAttrs } from "@/components/icons/utils"
import { createIconBase } from "../icon-base"
export const RockXGlyphIcon = createIconBase({
displayName: "RockXGlyphIcon",
viewBox: "0 0 32 32",
className: "size-[1em]",
...commonIconDefaultAttrs,
children: (
<>
<path
d="M19.0337 19.0337C20.7092 17.3583 20.7092 14.6417 19.0337 12.9663L7.32408 1.25661C5.64859 -0.41888 2.93207 -0.418859 1.25661 1.25661C-0.418859 2.93207 -0.41888 5.64859 1.25661 7.32408L9.93253 16L4.09187 21.8406C2.41641 23.5161 2.41639 26.2326 4.09187 27.9081C5.76736 29.5836 8.48388 29.5836 10.1593 27.9081L19.0337 19.0337Z"
opacity="0.6"
/>
<path
d="M12.9675 12.9662C11.2921 14.6417 11.2921 17.3582 12.9675 19.0337L24.6772 30.7433C26.3527 32.4188 29.0692 32.4188 30.7447 30.7433C32.4201 29.0679 32.4202 26.3513 30.7447 24.6759L22.0688 15.9999L27.9094 10.1593C29.5849 8.48382 29.5849 5.7673 27.9094 4.09181C26.2339 2.41633 23.5174 2.41635 21.8419 4.09181L12.9675 12.9662Z"
opacity="0.8"
/>
</>
),
})
- I reverted the className back to just
className: "size-[1em]",
- Realized the third path was an unnecessary duplicate, so I removed it
- Removed the
fill
attribute from the paths, sincefill="currentColor"
is already applied to everything via...commonIconDefaultAttrs
- Added
opacity
attributes (to specific paths) instead for this icon specifically, so give it an appearance of the two "arrows" intersecting with each other.
With those changes, you can see how this becomes nicely responsive in light and dark modes, similar to the other logos:
@wackerow Can you check and see what you think now? In the filament, I added the black with 0.5 opacity, giving more features for the black background, which works for both. The other I implemented your suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, nice job @rafagomes! Pulling this in
Congrats, your important contribution to this open-source project has earned you a GitPOAP! Be sure to join the Ethereum.org discord if you are interested in contributing further to the project or have any questions for the team. GitPOAP: 2024 Ethereum.org Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
@all-contributors please add @rafagomes for code |
I've put up a pull request to add @rafagomes! 🎉 |
This would be cool to add in staking limits as well. Do you need anything for that? |
Enhanced staking card component to meet Figma new layout. Adding social icons and changing new colors.
Description
Preview link
https://deploy-preview-14047--ethereumorg.netlify.app/en/staking/solo/#node-tools
Related Issue