Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Standardise on top margin for settings tabs #7727

Closed
wants to merge 1 commit into from

Conversation

sahilsaini1107
Copy link

@sahilsaini1107 sahilsaini1107 commented Feb 6, 2022

Fixes element-hq/element-web#20767

Signed-off-by: Sahil saini sohil170246@gmail.com.example.org


Here's what your changelog entry will look like:

🐛 Bug Fixes

@sahilsaini1107 sahilsaini1107 requested a review from a team as a code owner February 6, 2022 13:32
@SimonBrandner SimonBrandner added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 6, 2022
@SimonBrandner
Copy link
Contributor

Could you please add the following to the description

Fixes https://github.com/vector-im/element-web/issues/20767

and change the PR's name to something more descriptive as it will be a part of the changelog?

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Could this be somehow applied more globally instead of adding the code to each file? Some tabs also seem to have other than 10px padding or no padding, does this PR somehow fix those?

@sahilsaini1107
Copy link
Author

Could this be somehow applied more globally instead of adding the code to each file? Some tabs also seem to have other than 10px padding or no padding, does this PR somehow fix those?

okay i will check that and get back to you.

@sahilsaini1107
Copy link
Author

Could this be somehow applied more globally instead of adding the code to each file? Some tabs also seem to have other than 10px padding or no padding, does this PR somehow fix those?

may you please brief me which tabs are you talking about?

@t3chguy t3chguy changed the title Fixed:20767 Standardise on top margin for settings tabs Feb 7, 2022
@SimonBrandner
Copy link
Contributor

Could this be somehow applied more globally instead of adding the code to each file? Some tabs also seem to have other than 10px padding or no padding, does this PR somehow fix those?

may you please brief me which tabs are you talking about?

Some tabs (e.g. notifications) have margin other than 10 px (e.g. 24 px), how does setting the margin to 10 px fix this? Does it override some other CSS?

It's not clear to me how setting the margin to 10 px in some tabs fixes the problem, perhaps I am missing something?

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

To progress this pull request forward it would be great to have a similar screenshot to what was added to the original issue to see the before/after.

All the comments made by @SimonBrandner are also relevant, and generally speaking those styling rules should be applied more globally if we want to have more consistent spacing over time

I will request a design review once you have updated us with the screenshots

@sahilsaini1107
Copy link
Author

Add

okay i will try to apply css globally. and also provide you the screenshots

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardise on top margin for settings tabs
3 participants