Skip to content
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

style: align headers icons #975

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Conversation

ofirc77
Copy link
Collaborator

@ofirc77 ofirc77 commented Dec 13, 2024

Hi, this fix is for the following bug -
#974

It seems that in the header.css file the '.main-header' template with the field "justify-content" modified the behavior of the headers icons not to be aligned as expected. I change it to "flex-end" that verify that in each language we are using the headers icons bar will be at the end of line which means the other side as expected. Below there is an example of before and after the change.

Before change:
main_head_bar_in_chrome
main_head_bar_in_firefox

After change:
fix_head_bar_in_chrome
fix_head_bar_in_firefox

@ofirc77 ofirc77 requested a review from NoamGaash as a code owner December 13, 2024 15:46
@NoamGaash
Copy link
Member

Thanks!
Have you verified that it looks right on mobile devices?
(visual tests are not executed in the CI pipeline from forked repositories)

@NoamGaash NoamGaash changed the title Bug fix - align porperly headers icons style: align headers icons Dec 14, 2024
@ofirc77
Copy link
Collaborator Author

ofirc77 commented Dec 15, 2024

Yes. Checked it on mobile as well.
Hebrew view:
mobile_view_hebrew

English view:
mobile_view_english

@NoamGaash
Copy link
Member

In your PR preview, it seems different -
image
image

@ofirc77 ofirc77 force-pushed the align_headers_icons branch from b6e43f6 to aa6662f Compare December 16, 2024 21:50
@ofirc77
Copy link
Collaborator Author

ofirc77 commented Dec 16, 2024

Hi, updated the MR. Added specific tags for the mobile view. When I checked it as the screenshot you shared I saw the issue.
So I defined the CSS to order them differently when they are in mobile (also applied for dark mode )

pic1

pic2

@NoamGaash
Copy link
Member

Thanks @ofirc77 !

  1. could you please verify that you have pushed your code, and that it's passing the linter (npm run lint)?
  2. did you try to play with the horizontal size of the browser and see that it looks fine on every screen size?

@ofirc77 ofirc77 force-pushed the align_headers_icons branch from aa6662f to b75caaa Compare December 17, 2024 19:57
@ofirc77
Copy link
Collaborator Author

ofirc77 commented Dec 17, 2024

Yes. It passed the lint check and I increased the max-width to 750 to make sure also in horizontal it will look good as well. I played with it a little bit it seems that in some cases it will not function as expected.

@NoamGaash
Copy link
Member

@ofirc77 could you please make sure you have pushed your recent changes?
In the code, it seems like there's a missing ending curly brace (})

@NoamGaash
Copy link
Member

you can see it by visiting the commit statuses section of this pull request:
image

@ofirc77 ofirc77 force-pushed the align_headers_icons branch from b75caaa to c1fed48 Compare December 18, 2024 17:19
@ofirc77
Copy link
Collaborator Author

ofirc77 commented Dec 18, 2024

@NoamGaash updated the PR according to the project linter. It seems all tests are passing now

@@ -9,6 +9,16 @@
background: #303230;
}

@media (width <= 750px) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks. let's just be consistent with the mobile width

@media only screen and (width >= 768px) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ofirc77 ofirc77 force-pushed the align_headers_icons branch from c1ab2b9 to ff727b9 Compare December 19, 2024 17:02
Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! 🚌 💯 🥇

@ofirc77 ofirc77 merged commit 4b35700 into hasadna:main Dec 20, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants