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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix padding of sidebar without secondary actions #1362

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

raimund-schluessler
Copy link
Contributor

Fixes the right-padding of the sidebar title for sidebars (in compact mode || without figure) && without secondary actions 馃槈.

Before:
AppSidebar vue鈥搒ubtitle_null鈥搒tarred_false鈥揷ompact_false鈥揾eader_none鈥搒econdary_none鈥揺ditable_false-base

After:
AppSidebar vue鈥搒ubtitle_null鈥搒tarred_false鈥揷ompact_false鈥揾eader_none鈥搒econdary_none鈥揺ditable_false-actual

Signed-off-by: Raimund Schl眉脽ler <raimund.schluessler@mailbox.org>
@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component labels Aug 28, 2020
@raimund-schluessler raimund-schluessler self-assigned this Aug 28, 2020
@@ -522,6 +527,10 @@ $top-buttons-spacing: 6px;
// increase the padding to not overlap the menu
.app-sidebar-header__desc {
padding-right: #{$clickable-area * 2 + $top-buttons-spacing};

&.app-sidebar-header__desc--without-actions {
padding-right: #{$clickable-area + $top-buttons-spacing};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does without actions have more padding than the default ?
The css doesn't make sense there 馃

Without have 50px, default only have 6px ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's complicated. By default the menu action is part of the flow, so 6px is enough since the action menu itself will push the titles right border to the left. But as soon as you take the menu out of the flow (in case of no figure, or compact mode), the title needs a right-padding. And if there is no menu, the padding must be lower.

Feel free to change the CSS, the new base images in this PR will tell you if the CSS works. 馃槈

@skjnldsv
Copy link
Contributor

PIng? :)

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Oct 12, 2020

PIng? :)

To be honest, I don't really know how to further simplify / clarify the CSS. The visual appearance resulting is ok, as you see from the visual regression tests.

As said above, feel free to adjust the CSS structure. As long as you don't touch the base images and the regression tests still pass, I am all in 馃槈

@skjnldsv
Copy link
Contributor

No, I'm just super cautious now. 馃槄

@raimund-schluessler
Copy link
Contributor Author

No, I'm just super cautious now. 馃槄

I understand that. So should we merge it as is?

@raimund-schluessler raimund-schluessler merged commit c7f5c4e into master Oct 13, 2020
@raimund-schluessler raimund-schluessler deleted the fix/noid/sidebar-padding branch October 13, 2020 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants