Page layout change proposal #1524
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Really nice one! Just tested and I agree this is 100% better :) |
|
Awesome, thank you! I will adjust the paddings and then open the PR |
|
Alright all done, checked every page and sub page, should be good to go 🙂👍 |
Sendouc
reviewed
Oct 21, 2023
| @@ -12,7 +12,7 @@ export function SideNav() { | |||
| {navItems.map((item) => { | |||
| return ( | |||
| <Link | |||
| to={item.url} | |||
| to={`/${item.url}`} | |||
There was a problem hiding this comment.
Yeah I noticed these relative URL's have bene causing issues... Wonder what's the root cause, maybe Remix bug?
|
Very nice job! And good attention to detail with the scroll bar in this and the object damage calc pr :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed on the VODs page that the content clips into the sidebar before it reaches the breakpoint.
I was going to make a quick fix by adding some padding, but noticed the reason for this is that both navbar and sidebar do not take any actual space on the page due to having a fixed position.
By moving around some elements and changing them to be sticky its possible to have the same behavior while having the elements actually take up space.
This makes layouting easier and more consistent and avoids clipping. Both navbar and sidebar would also be more future proof to any changes since they can no longer mess with any page content.
It also has the added benefit of making the interaction with the footer a lot more natural!
If you look at the main content div you can see that it now respects the sidebar and navbar
I made the pull request a draft because in order to avoid clipping in the current layout, all pages have top paddings applied to them, which means I would have to go through every page and adjust the paddings to get the original look back, so I would like to get your thoughts on this first!