-
Notifications
You must be signed in to change notification settings - Fork 35
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
General Visual Overhaul #115
General Visual Overhaul #115
Conversation
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! I generally like the color tweaks, and the hovering behavior. Some of the icons are no-brainers, but a couple need some other maintainer input.
tailwind.config.cjs
Outdated
'lb-sky': '#6d98cc', | ||
'lb-purple': '#8a64e5', |
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.
Are these colors consistent with the brand guidelines? If you need a link, it should be in the #website channel on discord
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.
Oops forgot to reply to this one, the values themselves are consistent with it, however the naming scheme wasn't, I've since read the relevant sections of the branding guideline and addressed the required changes in 9d0589a.
src/styles/global.css
Outdated
color: #fff; | ||
border-radius: 100px; | ||
text-shadow: #000 1px 0 2px; | ||
transition: all 200ms; | ||
/* text-shadow: #000 1px 0 2px; */ |
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.
commented out?
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.
Yep, in my opinion text shadow does not good for the buttons. I forgot to completely remote it, so it was left commented out, but I've removed it in 802cac9.
src/styles/global.css
Outdated
} | ||
|
||
.news__box:hover { | ||
background-color: #8a64e533; |
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.
is this color consistent with the brand guidelines?
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.
Yep it does as far as I understand it, the only thing changed is the transparency value in the last 2 hex digits, because we want the hover effect to be subtle. However, to avoid confusion, I've added a separate variable for the transparent version of this violet in 9d0589a.
</div> | ||
<div id="header-donate" class="hidden lg:block"> | ||
<a href="https://donorbox.org/ladybird" class="btn btn--small"> | ||
<Icon name="mdi:donation-outline" class="size-5"/> |
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.
I'm not sold on this icon. @gmta ?
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.
I also agree that there might be better alternatives, do you have any suggestions you may like from Iconify? I have tried to pick the most fitting one from the MDI icon library, however there may be better options indeed either from MDI or different icon libraries found on Iconify.
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.
What about using a heart? Other projects use it as well, and mdi has a couple options for it.
@@ -138,7 +155,10 @@ const sponsors = [ | |||
<div class="mb-[64px] relative"> | |||
<div class="flex items-center mb-6"> | |||
<span class="block w-full h-[1px] bg-[#d3d2d6]" /> | |||
<span class="text-2xl font-semibold px-5">{tier.name}</span> | |||
<div class="flex flex-row items-center gap-2 px-5"> | |||
<Icon name="mdi:crown" class="size-8" style={`color: ${tier.color};`}/> |
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.
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.
I was trying to find a way to visually distinguish the tiers. The main issue is that visually they are all the same, nothing apart from the words themselves communicate which tier represents a higher donation rank. I thought the colored crown is a subtle way to highlight this difference while also adding some gentle visual flair. Do you have any ideas for other solutions we could explore, or perhaps we could polish this one by changing the colors and the icon used?
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.
What about coloured text for the name of the tier? combined maybe with a selection of icons, that would need to be discussed though.
Oh also please run |
Thank you for your feedback, I tried to address everything you mentioned in the conversation threads, and also ran |
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.
So, I had a chat with the other maintainers, and the consensus seems to be that they'd rather not consider or modify the visual style of the site at the moment.
In light of that, these changes should be backed out:
- The hover effect on buttons
- Adding the icons to buttons and sponsor tiers
- The border around the sponsors
The mild refactoring, css, and color changes are good and uncontroversial though.
Sorry for the lack of feedback on your posts in discord about the website style!
While I'm not exactly happy with this outcome, since from the external feedback I was able to gather these changes would overall make the site look better without significantly diverting from the existing style (and I was willing to work on polishing it), I respect your decision. The border around the sponsors wasn't my addition, that's also on the current production ladybird.org website. Unfortunately, dissecting my commits and cherry picking only the refactoring bits, then making a new PR is not something I'm interested in doing, sorry. In light of that, I'm closing this PR as to not pollute the PR list. If anyone is interested in integrating those specific changes, I don't mind at all. If you're still interested in discussing this, feel free to ping me on Discord. |
This PR aims to improve the style of the current ladybird.org website by incorporating a bunch of smaller changes that I believe have a nice benefit overall. The commit messages describe pretty well what has been changed, however here's a simplified list:
Visual
Misc
Demo: https://ladybird-org-taupe.vercel.app/