replace SideNav 'closed' class to 'open'#8389
replace SideNav 'closed' class to 'open'#8389AndreJesusBrito wants to merge 3 commits intoFreeTubeApp:developmentfrom
Conversation
github-actions
bot
added
the
PR: waiting for review
There was a problem hiding this comment.
This will conflict with #8365 and as that already has two approvals lets get that merged first, please also make sure this doesn't regress any of the RTL/LTR fixes in that pull request after it is merged:
- LTR channel name + LTR locale -> name is displayed LTR
- LTR channel name + RTL locale -> name is displayed LTR
- RTL channel name + LTR locale -> name is displayed RTL
- RTL channel name + RTL locale -> name is displayed RTL
absidue
added
PR/Issue: dependent
and removed
PR: waiting for review
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
absidue
added
PR: merge conflicts / rebase needed
and removed
PR: merge conflicts / rebase needed
PR/Issue: dependent
labels
The 'open' state now becomes the default for styling. This has three advantages: - This makes it consistent with the rest of the application logic (isOpen) - Simplifies styling. The open styles can now be done for desktop easily with a media query - Fixes FreeTubeApp#6091 [Bug]: Theme Settings > Expand side bar by default is affecting the bottom navigation for mobile devices
Head branch was pushed to by a user without write access
AndreJesusBrito
force-pushed
the
sidenav-improv
branch
from
fc6476e to
0c38614
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
@absidue I have checked and it's working correctly, the only thing I have to change is remove the margin at the right RTL locale: LTR locale with margin: LTR locale without margin: I have also rebased my patch on top of the current development branch |
|
Feel free to change whatever you need to change to make it look and work the same (on all screen sizes) as it did before this PR, as the pull request description says that there shouldn't be any visual changes. |
Head branch was pushed to by a user without write access
|
@AndreJesusBrito is this ready to review again? |
|
@efb4f5ff-1298-471a-8973-3d47447115dc yes, sorry, I should have commented that was ready. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
efb4f5ff-1298-471a-8973-3d47447115dc
added
PR: waiting for review
PikachuEXE
left a comment
PikachuEXE
left a comment
There was a problem hiding this comment.
I tested briefly but I got no idea what I should watch out for
| margin-inline-start: 0; | ||
| inline-size: 100%; | ||
| display: block; | ||
| margin-block-end: 0; |
There was a problem hiding this comment.
Can margin rules be grouped together?
| margin-inline-start: 0; | ||
| font-size: 11px; | ||
| margin-block-end: 0; |
There was a problem hiding this comment.
Same for margin, probably more in this file not pointing out more
|
@PikachuEXE did you see linked issue #6091? |
|
#6091 is tested I mean what else :) |
|
Nothing i think
|
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
4 participants
Pull Request Type
Related issue
closes #6091
Description
This pull request is mostly a refactor and doesn't have any visual or functional effect besides fixing the mentioned issue.
The 'open' state now becomes the default for styling. This has three advantages:
Theme Settings > Expand side bar by defaultis affecting the bottom navigation for mobile devices #6091 [Bug]: Theme Settings > Expand side bar by default is affecting the bottom navigation for mobile devicesTesting
Ensure that the interface behaves as before when the app resolution (media queries), nav bar open toggle and the Theme > Hide Side Bar Labels option vary.
Desktop