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 adjustments #263

Merged
merged 3 commits into from
Aug 18, 2022
Merged

Style adjustments #263

merged 3 commits into from
Aug 18, 2022

Conversation

chukarave
Copy link
Contributor

The linter rule does not allow using 'rem' as a unit,
i've disabled it.
font-size for the copyright message should be 13px, the
rem in this case is relativly calculated from 1rem which is 16px.
margins between elements adapted to represent 16px as well.
The main content wrapper will get a top margin of 0.5rem
so that the gap between search-exiting or anonymous edit message
and the main form always remains 1.5rem.

Bug: T312652

The linter rule does not allow using 'rem' as a unit,
i've disabled it.
font-size for the copyright message should be 13px, the
rem in this case is relativly calculated from 1rem which is 16px.
margins between elements adapted to represent 16px as well.
The main content wrapper will get a top margin of 0.5rem
so that the gap between search-exiting or anonymous edit message
and the main form always remains 1.5rem.

Bug: T312652
@sai-san
Copy link

sai-san commented Aug 16, 2022

✅ The font-size of the copyright message is at least 13px
✅ The elements within the form have a default spacing of 1rem (generally 16px) between them**
❌ I'm not sure that the main content wrapper is displaying a margin of 0.5rem. The gap between search-exiting or anonymous edit message and the main form seems to be 1rem at the moment.

**I noticed that the copyright message still has a margin of 1em applied to it. This doesn't cause any problems because it overlaps with the 1rem margin applied to all elements. It might be a good idea to remove that just for consistency.

Screenshot 2022-08-16 at 11 24 27

Tested on Chrome 104, Safari 15.6 and Firefox 103 on macOS Monterey.

@chukarave
Copy link
Contributor Author

@sai-san

  • the main content wrapper is not displaying the 0.5 margin here, it's part of the Gerrit patch as it's set from outside the Vue application.
  • Do you mean setting the copyright message margin explicitly to rem or removing it altogether?

@sai-san
Copy link

sai-san commented Aug 17, 2022

Answering inline, @chukarave.

  • the main content wrapper is not displaying the 0.5 margin here, it's part of the Gerrit patch as it's set from outside the Vue application.

Got it! Is there a way I can check the Gerrit patch? I'm not sure where to find a Netlify link.

  • Do you mean setting the copyright message margin explicitly to rem or removing it altogether?

I think that removing the bottom margin makes sense, since it's not contributing to spacing the layout. We might need to keep the 1rem margin at the top to ensure that the last field and the message have proper separation between them. This approach would allow us to maintain proper spacing between the last field and the button if the text were removed.

@chukarave
Copy link
Contributor Author

@sai-san

Is there a way I can check the Gerrit patch? I'm not sure where to find a Netlify link.

as we discussed, this is not part of the Vue app so will be visible on Beta once merged

@chukarave chukarave requested a review from sai-san August 18, 2022 10:00
Copy link

@sai-san sai-san left a comment

Choose a reason for hiding this comment

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

All looking great! Thanks, @chukarave 💯

@chukarave chukarave merged commit 1917ce9 into main Aug 18, 2022
@chukarave chukarave deleted the design-improvements branch August 18, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants