-
Notifications
You must be signed in to change notification settings - Fork 148
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce page-header for dashboard-title and settings #238
Conversation
This reverts commit 811bc1f.
for more information, see https://pre-commit.ci
This reverts commit 811bc1f.
for more information, see https://pre-commit.ci
3926b9b
to
aaebcee
Compare
…insey/vizro into dev/dashboard_title_position
Starting having a look. I tried to understand these changes in the context of #205, but it was a little tricky to do as the graphic does not seem to match anymore. Do you think it makes sense to provide an update of that? |
Sure, I've updated the PR description 👍 In general, it's best to check the attached Miro links, I've added screenshots of the layout everywhere so you can imagine how it will look like. This just adds another layer of outer containers and moves the dashboard title, settings and logo in the page-header container. The rest remains the same and will live in the page-main container. The settings container will move into the page-header or right-header depending whether the page-header exists or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! 🚀 The graphic really helped, I think its good to have it in a PR for public view 🙏
vizro-core/changelog.d/20240103_210035_huong_li_nguyen_dashboard_title_position.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had one small comment, but otherwise looks good. I like the new design with the dashboard title separated (makes much more sense and the hierarchy is clear). 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great I think, definitely feels like a good solution to me 👍
As per a comment left on your other PR, I do wonder whether we should be changing some of these div
s to more semantic elements, but it's really not important and definitely not worth doing now.
Description
Context: There are currently some issues with the dashboard-title position:
Introduction of
page-header
:Above solution required several CSS changes such that it looks good with the page-header as well, see code for more details
Layout Screenshots
Take a look at the Miro board - there are several screenshots that show you how it looks now here
Container Structure
https://miro.com/app/board/o9J_ldRDmCQ=/?moveToWidget=3458764572914015668&cot=14
Screenshot
In the Miro board there are more screenshots - adding a selected one here:
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":