-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
@@ -6,20 +6,29 @@ | |||
<header | |||
class="flex-grow w-full lg:w-auto lg:min-w-[32rem] xl:min-w-[64rem] box-border flex flex-col justify-between lg:justify-center" | |||
> | |||
<VLogoButton class="lg:hidden ms-3" :auto-resize-logo="false" /> | |||
<VLogoButton |
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.
Neither header nor VLogoButton
have a color class, so the color of the logo becomes black instead of dark charcoal.
Do we really need to change the logo direction? I couldn't find anything specifically on this, but Ahmad Shadeed's RTL-styling blog uses the same logomark for RTL, without changing direction, for Smashing Magazine: https://rtlstyling.com/posts/rtl-styling#3.-differences-in-word-sizes-between-languages . |
@obulat I don't think we necessary do, but in the case of the homepage, the LTR design looks very unbalanced and awkward with out a flipped logo mark. There's no sensible way to have the logo mark "hang" off the leading edge of the page like it does on the LTR version. If @panchovm has any ideas of how to do this without flipping the logo and brand mark, then we can implement that. But as it stands I couldn't come up with anything that looked decent. |
Oh, I didn't realize that this is mainly a solution to fix the "hanging" part of the large screen logo, thank you for explaining! By the way, the color of the logo in the homepage header on screens smaller than |
I love ❤️ @sarayourfriend's solution of flipping the logo. It embraces the RTL logic as the most prominent use of Openverse will be through the symbol rather than symbol and wordmark. Therefore, switching positions is a great idea. The reading logic is tricky as the |
853087e
to
ed5fb5a
Compare
@obulat thanks for catching that. How are you able to tell by the way? I can't figure out any way to determine the fill color of the SVG and visually I can't tell the difference between the |
I can see the difference in colors. I guess working closely with @panchovm on the header fixes helped me learn to pay more attention to colors and spacings 😀 |
I envy your eyes then! I looked very hard and close and really couldn't tell the difference 😆 Thanks for lending your eyes 🙏 |
Although let me say that @obulat's eagle eyes date longer than the header work. |
Monitor color profiles could also play a major role here 😆 |
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.
This looks great, I do see one visual regression. Probably as a consequence of separating the logo text and icon, the logo text is no longer aligned with the rest of the content:
after
before
The simplest fix might be for the logo text to be in the same container as the other content, and for the logo icon to be absolutely positioned relative to that container.
Ah, that's a much wider browser window configuration than I'm able to achieve with my current set up 😅 I definitely ran into similar problems but didn't realize the margins would keep moving as the view got even larger. I'll try the absolute positioning trick you recomended. |
@zackkrida updated, please let me know if this works for you! |
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 perfect!
Fixes
Fixes #765 by @sarayourfriend
Description
Fixes the homepage RTL problems.
Testing Instructions
Load up this branch and run
pnpm dev
./
and verify that the homepage looks good at all screen widths/ar
and verify that the homepage looks good at all screen widths and that text is aligned/404
and verify that the logo and brand mark look good/ar/404
and verify that the logo and brand mark look goodChecklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin