Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable dashboard.title and add div with package version #31

Merged
merged 11 commits into from
Sep 13, 2023
Merged

Enable dashboard.title and add div with package version #31

merged 11 commits into from
Sep 13, 2023

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Sep 12, 2023 •
edited

Description

  • Enable dashboard.title and add tests (if no title is specified, the navigation panel will just move up)
  • Add hidden div with Vizro version in dashboard container

Minor changes added:

  • Clean up Alert model (still private)
  • Update CSS of Accordions (to align with page title horizontally and controls title vertically)
  • Add div for controls only if there is content

Screenshot

Hidden div with version:
Screenshot 2023-09-12 at 16 57 54

With dashboard.title:
Screenshot 2023-09-12 at 17 42 50

Without dashboard.title:
Screenshot 2023-09-12 at 17 38 57

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added the PR number to the change description in the changelog fragment, e.g. Enable feature XXX ([#1](https://github.com/mckinsey/vizro/pull/1)) (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@huong-li-nguyen huong-li-nguyen marked this pull request as ready for review September 12, 2023 15:47
@huong-li-nguyen huong-li-nguyen self-assigned this Sep 12, 2023
@huong-li-nguyen huong-li-nguyen added Issue: Feature Request 🤓 Issue/PR contains a feature request or is based on a feature request Status: Ready for Review ☑️ Issue/PR is ready for review - all tests have passed labels Sep 12, 2023
@huong-li-nguyen huong-li-nguyen changed the title Enable dashboard.title and add hidden div with version Enable dashboard.title and add hidden div with package version Sep 12, 2023
@huong-li-nguyen huong-li-nguyen changed the title Enable dashboard.title and add hidden div with package version Enable dashboard.title and add div with package version Sep 13, 2023
nadijagraca
nadijagraca previously approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@nadijagraca nadijagraca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I just have two minor comments regarding css, but good to merge.

Second comment:

  • Page title and Dashboard title are not 100% inline. Upon inspection, it seems that "height" on right side header is causing issues. If you remove the height they align perfectly.

vizro-core/src/vizro/static/css/accordion.css Show resolved Hide resolved
maxschulz-COL
maxschulz-COL requested changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Have a few suggestions regarding the docs mostly

vizro-core/docs/pages/user_guides/dashboard.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/dashboard.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/dashboard.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/dashboard.md Show resolved Hide resolved
vizro-core/changelog.d/20230912_165919_huong_li_nguyen_enable_dashboard_title.md Outdated Show resolved Hide resolved
chiara-sophie
chiara-sophie previously approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@chiara-sophie chiara-sophie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍 some minor comments

vizro-core/docs/pages/user_guides/dashboard.md Outdated Show resolved Hide resolved
vizro-core/schemas/0.1.1.dev0.json Outdated Show resolved Hide resolved
vizro-core/schemas/0.1.1.dev0.json Outdated Show resolved Hide resolved
vizro-core/src/vizro/models/_dashboard.py Outdated Show resolved Hide resolved
vizro-core/src/vizro/models/_dashboard.py Outdated Show resolved Hide resolved
@huong-li-nguyen
Copy link
Contributor Author

Looks good. I just have two minor comments regarding css, but good to merge.

Second comment:

  • Page title and Dashboard title are not 100% inline. Upon inspection, it seems that "height" on right side header is causing issues. If you remove the height they align perfectly.

Good catch! Let me actually add the height to the app title header as well, because the height is one of the design specifications.

nadijagraca
nadijagraca approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@nadijagraca nadijagraca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@huong-li-nguyen huong-li-nguyen merged commit 1bfae30 into main Sep 13, 2023
16 checks passed
@huong-li-nguyen huong-li-nguyen deleted the feat/enable_dashboard_title branch September 13, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxschulz-COL maxschulz-COL maxschulz-COL approved these changes

@nadijagraca nadijagraca nadijagraca approved these changes

@lingyielia lingyielia Awaiting requested review from lingyielia

@petar-qb petar-qb Awaiting requested review from petar-qb

@Anna-Xiong Anna-Xiong Awaiting requested review from Anna-Xiong

@chiara-sophie chiara-sophie Awaiting requested review from chiara-sophie

Assignees

@huong-li-nguyen huong-li-nguyen

Labels
Issue: Feature Request 🤓 Issue/PR contains a feature request or is based on a feature request Status: Ready for Review ☑️ Issue/PR is ready for review - all tests have passed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@huong-li-nguyen @maxschulz-COL @chiara-sophie @nadijagraca