Moved the links to the sidebar #236#246
Moved the links to the sidebar #236#246FedericoTartarini merged 23 commits intoCenterForTheBuiltEnvironment:developmentfrom
Conversation
…antine-components - updated dash and dash-mantine-components - moved the navigation and document, global, and IP to the sidebar - modified the spec.cy.js - updated the tabs.css and layout.css
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FedericoTartarini
left a comment
There was a problem hiding this comment.
I left a few comments, I did not review the whole code. In summary do not use CSS unless 100% needed, it is generally not needed since mantine components are well designed. Also do not simply replace Div with Box, but instead check if the Div is needed at all, if not just remove it. I am sure that more than 50% of them can be removed by using the right mantine components like Stack, Group, Grid, ...
- used Dash-Mantine components styles API - formatted the code via using ruff and black
|
After thorough investigation, we have traced the issue back to the We discovered that replacing the To ensure stability, we have kept this single use of Please let us know how you’d prefer to proceed with this specific case. |
|
@Sallie00 I am happy with your decision. please keep those html.A components when necessary and otherwise breaking the code. I like this since we can still use the old test framework. Please tag me here once the pull request is complete so I will review it. |
|
Hi @FedericoTartarini , The pull request is now ready for your review. |
There was a problem hiding this comment.
@Sallie00 I really like the work; however, I noticed a major issue. The navbar is closed by default, and it will be very confusing for current and future users to find the tabs. The navbar, to work well:
- needs to be open by default
- it should not be going over the page but shift the page as I mentioned before, see this website as an example https://fly-log.netlify.app/
- this is very important also because it is a bit annoying to have to open the navbar each time the user wants to change the page.
- the logo in the title of the navbar is not showing
- reduce sligthly the spacing between the links in the navbar.
code review
Very good work I'm very happy with the progress. However, I think they code can be further simplified. For example, I think you have too many nested components. Please review all my comments carefully. I am a little bit worried because sometimes you change the argument of the input or of the output in the callback. Please make sure that this is what you want to do. Please also try to use as much as possible the default component as provided by Mantine. Using the default will ensure that the website is consistent and beautiful.
|
@FedericoTartarini Version 1The first version uses the Full AppShell Layout https://py.cafe/dash.mantine.components/AppShell-with-all-elements, divided into header, navbar, main, and footer sections. The main section is scrollable, though this may not be readily apparent in my screenshot. Simultaneously, clicking the burger button controls the display and concealment of the sidebar, functioning identically to the demonstration in Dash Mantine. By default, it remains expanded. Version 2The secondary version uses Collapsible AppShell Layout https://py.cafe/dash.mantine.components/Collapsible-navbar-on-both-desktop-and-moble, divided into header, navbar, and main sections. The main section includes the original layout's main and footer elements and is also scrollable. The function of the burger button remains unchanged from Version 1. Which version do you prefer? |
|
@LeoLuosifen I am not fully understanding what the difference is between v1 and v2. Please explain the key differences and how this will affect the user experience of Clima. |
|
@FedericoTartarini The entire page appears somewhat small in v1, as my computer screen is rather compact. While it might look better on a larger display, this may not suit all users. The v2 more closely looks like the effect shown in the link you provided https://fly-log.netlify.app/. |
|
I want to recreate the same effect as in https://fly-log.netlify.app/. please also note that the navbar width can be slighlty reduced since it does not need to be so wide. If version 2 is similar to https://fly-log.netlify.app/ then we can implement version 2 |
- optimized the original simple dmc components - removed the element ids are no longer needed - removed the unnecessary css style in css files - removed page_icon.py, moved to layout.py and renamed class name to NavbarIcons - updated spec.cy.js - reformatted the code
…using default style
|
@FedericoTartarini |
Removed unnecessary Stack component to streamline the layout and improve readability. Also optimized spacing in the layout by removing redundant CSS properties.
Replace Box with Stack for improved layout structure and remove unnecessary style properties to simplify the code.
…raphs Consolidate container and stack components for improved readability and maintainability. Adjust grid column spans for responsive design.
Clean up the layout by eliminating redundant margin and padding properties in the title_with_tooltip function to streamline the component's styling.
Updated the layout function to utilize the Stack component instead of Container, improving the structure and alignment of child elements.
Refactor the layout function for tab four by removing the old definition and ensuring the layout is consistently defined using the Stack component.
Replace dcc.Loading with dmc.Center for improved layout consistency. Remove unused CSS styles related to tabs for cleaner codebase.
Refactor the layout function to use a Stack component instead of Box, improving the structure and consistency of the layout in tab six.
Eliminate fixed y-axis range to allow for dynamic scaling based on data.
Clean up the code by removing hardcoded width styles from various dropdowns to improve layout consistency and maintainability.
Reorganize the layout code in section one for better readability and maintainability. This includes consistent indentation and formatting adjustments.
|
Very good work, I really like what you have done. Please all carefully review all my changes. This will be an opportunity for you to learn and see how css could have been removed and the code could have been much further simplified. I did not implemented all the changes I wanted but as you can see we could much furhter improve the code and avoiding nesting components. I will need to stop here for today and I will accept the pull request for now, but note that we can improve it much further. |
60214b6
into
CenterForTheBuiltEnvironment:development






Summary
This PR refactors the layout by moving the navigation links previously displayed in the top banner into a collapsible sidebar (
dmc.Drawer) on the left side of the page. It also updates the end-to-end test filespec.cy.jsto reflect the new layout and interaction behavior.Changes
dash-mantine-components(dmc.NavLink,dmc.SegmentedControl, etc.) to build the new sidebar.spec.cy.jstest code to match the new layout structure: