-
Notifications
You must be signed in to change notification settings - Fork 154
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
[Demo] Add pycafe link and plotly code to ViViVo #824
Conversation
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2024-10-30 11:23:19 UTC Link: vizro-core/examples/dev/ |
max-height: 500px; | ||
overflow: auto; | ||
padding: 1rem; | ||
position: relative; | ||
} | ||
|
||
.code-clipboard-container .pycafe-link, |
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.
To be discussed with @huong-li-nguyen: should we do this styling for other buttons. e.g. the one on the 404 page looks a bit weird I think.
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.
Happy to get @huong-li-nguyen opinion on these UI changes. The new functionalities are super super cool, but I still think that there's a room for improvement for the UI changes.
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.
Also, did we consider making the code-clipboard-container
always stretched to the bottom of the page so it's always aligned with the bottom of the chart?
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 think @huong-li-nguyen spent a while playing around with adjusting the size and stretching the code clipboard container and decided the current solution is best, but let's check when she gets back.
What else do you think could be improved in terms of UI?
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 can’t say for sure, but here are a few ideas we might consider:
- Moving the button between the tab selection and clipboard icon, placing it logically outside the "copy-button" area.
- Adding a small PyCafe icon within the button text (and possibly shortening the text if it improves clarity).
- Including a link icon 🔗 (or a similar symbol) to indicate that clicking the button will open a new browser tab.
These are minor suggestions, so feel free to disregard if they don’t quite fit! 😄
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.
All good points I think! I will unresolve this conversation after merging so we can revisit when @huong-li-nguyen is back. My ideal UI here would actually probably not be tabs at all but instead a toggle switch where it highlights/unhighlights the Vizro code as you toggle on/off. The current solution is really just meant as a relatively quick and easy addition to the UI that works well enough for now rather than a completely polished one.
- I tried this and found it looked worse rather than better 😅
- I did play around with a pycafe icon on the button for a while and couldn't get anything that worked well with our colors. I also tried using the coffee emoji which looks pretty similar but didn't like it much.
- I've put in an "opens in new tab" icon and it looks good I think!
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 awesome 🚀 and probably something we should add to our dashboard-vizro-ai app as well (as we already discussed if I'm correct).
Also @maxschulz-COL thanks for the pycafe links in the PR comment as it saved my time for testing the changes 🥇
max-height: 500px; | ||
overflow: auto; | ||
padding: 1rem; | ||
position: relative; | ||
} | ||
|
||
.code-clipboard-container .pycafe-link, |
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.
Happy to get @huong-li-nguyen opinion on these UI changes. The new functionalities are super super cool, but I still think that there's a room for improvement for the UI changes.
max-height: 500px; | ||
overflow: auto; | ||
padding: 1rem; | ||
position: relative; | ||
} | ||
|
||
.code-clipboard-container .pycafe-link, |
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.
Also, did we consider making the code-clipboard-container
always stretched to the bottom of the page so it's always aligned with the bottom of the chart?
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.
Approved from my side, very cool addition! Really like Petar's suggestion of preventing duplicate code if possible, but otherwise looks great.
Does the plotly figure maybe need a fig.show()
statement? Or are we assuming people would use this in a notebook?
vizro-core/examples/visual-vocabulary/pages/examples/magnitude_column.py
Outdated
Show resolved
Hide resolved
…_column.py Co-authored-by: Maximilian Schulz <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Lingyi Zhang <[email protected]>
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.
👍
Thanks for the reviews everyone! I've rolled out the changes across the dashboard and checked through everything. I'll give it one final check over and would then like to merge, update on HF, and update our post on plotly forums. @huong-li-nguyen and I can come back to some of the minor comments and questions raised in this PR when she's back. I got halfway through de-deduplicating things and then remembered why I didn't do this before. The current structure is repetitive but makes it very explicit how the eventual app is built as a vizro app. While we still have a lot of pages unfinished and will hopefully get some other people to add to the dashboard I'd like to keep it explicit so that contributors "feel" like they're using Vizro. e.g. being able to copy and paste
Yeah, I considered this and decided to not add it because we don't really know how the figure would be consumed, whether it would be in a notebook or in a Dash/other sort of dashboard or what. |
for more information, see https://pre-commit.ci
This reverts commit 0aa6372.
This reverts commit 26ca1bf.
for more information, see https://pre-commit.ci
This reverts commit 859df57.
# Conflicts: # vizro-core/examples/scratch_dev/README2.md
Description
Still WIP with some code tidying needed (e.g. to sort imports). So far only the Magnitude pages have been done, so take a look at those (available immediately by clicking the link on the comment below!! 🚀 ). I'm raising for review already so you can take a look and see what you think before we implement changes across the whole dashboard.
Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":