refactor: align mobile sidebar close button to right side #174
Conversation
lex111
commented
Sep 1, 2021
•
lex111
commented
facebook-github-bot
added
the
CLA Signed
Dist CSS Diffdiff --git a/packages/core/dist/css/default/default.css b/packages/core/dist-branch/css/default/default.css
index a41c985..42a1145 100644
--- a/packages/core/dist/css/default/default.css
+++ b/packages/core/dist-branch/css/default/default.css
@@ -341,7 +341,7 @@
--ifm-navbar-search-input-color: var(--ifm-color-emphasis-800);
--ifm-navbar-search-input-placeholder-color: var(--ifm-color-emphasis-500);
--ifm-navbar-search-input-icon: url('data:image/svg+xml;utf8,<svg fill="currentColor" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" height="16px" width="16px"><path d="M6.02945,10.20327a4.17382,4.17382,0,1,1,4.17382-4.17382A4.15609,4.15609,0,0,1,6.02945,10.20327Zm9.69195,4.2199L10.8989,9.59979A5.88021,5.88021,0,0,0,12.058,6.02856,6.00467,6.00467,0,1,0,9.59979,10.8989l4.82338,4.82338a.89729.89729,0,0,0,1.29912,0,.89749.89749,0,0,0-.00087-1.29909Z" /></svg>');
- --ifm-navbar-sidebar-width: 80vw;
+ --ifm-navbar-sidebar-width: 83vw;
--ifm-pagination-border-radius: calc(
var(--ifm-global-radius) * var(--ifm-pagination-size-multiplier)
);
@@ -2706,7 +2706,8 @@ html[data-theme='dark'],
}
.navbar-sidebar__close {
- margin: 0 0.8rem 0 0.3rem;
+ display: flex;
+ margin-left: auto;
}
/** |
|
Size Change: +84 B (0%) Total Size: 565 kB
ℹ️ View Unchanged
|
|
✔️ Deploy Preview for infima ready! 🔨 Explore the source changes: fbf1726 🔍 Inspect the deploy log: https://app.netlify.com/sites/infima/deploys/612f638b91341e0007472aee 😎 Browse the preview: https://deploy-preview-174--infima.netlify.app/demo |
| @@ -284,7 +284,8 @@ html[data-theme='dark'], | |||
| } | |||
|
|
|||
| &__close { | |||
| margin: 0 0.8rem 0 0.3rem; | |||
| display: flex; | |||
| margin-left: auto; | |||
There was a problem hiding this comment.
As brand and close are already in a flex row, I'm not too fan of using margin-left: auto.
Also why use display: flex here?
As we eventually plan to allow configuring the icon position later, it may be a good idea to account for that today in Infima, as it could later lead to an Infima breaking change?
What about this instead:
&__brand {
fiex: 1;
margin-right: 1rem; // can now be removed? brand with no icon should take all navbar space
}
&__right {
flex: 0;
margin-left: 1rem;
}
&__left {
flex: 0;
margin-right: 1rem;
}It would be more future proof to add icons on both sides (including multiple icons)
In my opinion, we could even add this to Infima after it's already been in use in Docusaurus for a while, battle-tested and it's considered stable enough. This way we ensure there are limited breaking changes in Infima. It doesn't mean there won't be breaking changes in Docusaurus, but Docusaurus classes are not officially documented and harder to target with custom CSS)
|
`margin-left: auto` is just quick way to move flex item to the right
side. I could have used `flex: 1; justify-content: flex-end` instead
but does it really matter?
`display: flex` is needed to vertical center "x" icon correctly
(missed it in the first PR).
I wouldn't separate current layout on right and left columns now,
maybe we should wait until there is a request for such a feature from
our users? This PR contains min set of changes needed to add close
button in Docusaurus.
2021-09-01 15:56 GMT+03:00, Sébastien Lorber ***@***.***>:
… @slorber requested changes on this pull request.
> @@ -284,7 +284,8 @@ html[data-theme='dark'],
}
&__close {
- margin: 0 0.8rem 0 0.3rem;
+ display: flex;
+ margin-left: auto;
As brand and close are already in a flex row, I'm not too fan of using
`margin-left: auto`.
Also why use `display: flex` here?
As we eventually plan to allow configuring the icon position later, it may
be a good idea to account for that today in Infima, as it could later lead
to an Infima breaking change?
What about this instead:
```css
&__brand {
fiex: 1;
margin-right: 1rem; // can now be removed? brand with no icon should
take all navbar space
}
&__right {
flex: 0;
margin-left: 1rem;
}
&__left {
flex: 0;
margin-right: 1rem;
}
```
It would be more future proof to add icons on both sides (including multiple
icons)
---
In my opinion, we could even add this to Infima after it's already been in
use in Docusaurus for a while, battle-tested and it's considered stable
enough. This way we ensure there are limited breaking changes in Infima. It
doesn't mean there won't be breaking changes in Docusaurus, but Docusaurus
classes are not officially documented and harder to target with custom CSS)
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#174 (review)
|
I don't think it's a good practice to mix CSS layout systems when you can stick to one.
The flex row already align row items to the center. To fix it properly, you either have to:
Agree, let's do that later Actually, I noticed other issues that are a bit annoying and only happen at certain viewport widths:
As I'd like to fix this asap for the RN website, I'll take over this PR and finish it Edit: for weird reasons I can't reproduce this bug very consistently, might be a local chrome devtools issue 🤷 (editing meta viewport seems to re-render fine) |
|
Ok, so I'm going to merge this as-is because unfortunately to do what I want would require a deeper refactoring. |
|
Alright, thanks. |
