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.

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

817 dashboard move menu #830

Merged
merged 16 commits into from
Aug 6, 2024
Merged

817 dashboard move menu #830

merged 16 commits into from
Aug 6, 2024

Conversation

laurasootes
Copy link
Contributor

This PR moves the menu from the left to the top of the page. This deviates from streamlits defaults, and requires the use of dynamic page imports. Also the ‘original menu’ has to be hidden in the settings.

These adjustments cause the changed parameters in each page to be conserved between pages. To solve this I added page-specific keys to the variable elements, which are reset when you switch pages.

Also added icons to the menu items :)

Solves #817

@laurasootes laurasootes requested a review from elboyran July 30, 2024 15:22
@laurasootes laurasootes linked an issue Jul 30, 2024 that may be closed by this pull request
@laurasootes laurasootes requested review from SarahAlidoost and removed request for elboyran July 31, 2024 09:40
Copy link
Contributor

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@laurasootes nice work 👍 I liked the small icons next to each menu option. I was able to run the dashboard, but some of the tests in test_dashbord fail on expect(page).to_have_title(...). and also some minor comments on changed files.

@laurasootes
Copy link
Contributor Author

@SarahAlidoost thanks for your review! I implemented your code suggestions and the linting now passes.

I looked at the error you mention for the dashboard titles. I do not know why they would arise. Also I do not get them locally and don’t see them in the logs in the github actions either so I am a bit puzzled. Could you maybe try yo run them again?

I added your comment for the pages section on the home page to #816, because I think there will be some more discussion on what should and should not be on the home page

@SarahAlidoost
Copy link
Contributor

@SarahAlidoost thanks for your review! I implemented your code suggestions and the linting now passes.

I looked at the error you mention for the dashboard titles. I do not know why they would arise. Also I do not get them locally and don’t see them in the logs in the github actions either so I am a bit puzzled. Could you maybe try yo run them again?

You are right. Perhaps something related to running the tests locally on my side. I ran the dashboard test as pytest -v -m dashboard --dashboard on the main branch too and got the same error. Somehow the titles of pages are for example "Text · Streamlit" instead "Text" on my side 😄 . As you don't get the errors locally, I approved this PR. The failed GitHub actions are due to issue #837 .

@laurasootes laurasootes merged commit f324602 into main Aug 6, 2024
5 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard: move menu
2 participants