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

[POC] Add chart gallery dashboard #559

Merged
merged 131 commits into from
Jul 29, 2024
Merged

[POC] Add chart gallery dashboard #559

merged 131 commits into from
Jul 29, 2024

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Jul 1, 2024

Description

  • Set up initial layout
  • Added content and code to the 12 most commonly used charts: Bar, Column, Line, Scatter, Pie, Donut, Boxplot, Violin, Butterfly, Sankey, Choropleth, Treemap
  • Added CSS to get as close as possible to the Github code formatting colors

Note: Don't distribute this POC just yet. We plan to publish it internally and externally when the majority of content is filled :)

Also ignore the large number of code line changes, it's mostly the code for the svg files 😄 And lots of description text for the charts, so hopefully it doesn't look too daunting anymore.

Screenshot

Screenshot 2024-07-17 at 16 41 53 Screenshot 2024-07-17 at 16 41 45

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • 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.

@pre-commit-ci pre-commit-ci bot temporarily deployed to circleci_secrets July 1, 2024 15:25 Inactive
@pre-commit-ci pre-commit-ci bot temporarily deployed to circleci_secrets July 3, 2024 12:11 Inactive
@pre-commit-ci pre-commit-ci bot temporarily deployed to circleci_secrets July 3, 2024 16:26 Inactive
@pre-commit-ci pre-commit-ci bot temporarily deployed to circleci_secrets July 25, 2024 12:36 Inactive
@huong-li-nguyen
Copy link
Contributor Author

Yeah, I noticed that - I think it's not something that I broke but just because there's more content on that page now. I've nearly finished the refactoring and then we can fix that afterwards 🙂

I've fixed it already 👍

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Sorry my review took so long - I wanted to give this the full love it deserves because it's really an amazing dashboard ⭐ 💯 This is going to be a looooong review comment also, sorry...

I've checked through all the pages and all the graph are still in the right categories and all the links etc. work so no need for you to check that.

I ended up doing a bit of refactoring because I was having quite a good time 😁 Hopefully the commit comments make it clear what's changed. I'm happy to discuss/explain more if you like, but in summary:

  • chart_groups now provides a central source of truth on page groupings, which pages are incomplete, etc. so there's less duplication and potential inconsistencies and easier contribution process
  • IncompletePage class which simplifies things quite a bit since it can be handled the same way as vm.Page
  • example code split off into separate files to make it easier to edit and benefit from linting
  • example code is run through black with customisable line length and other options so it doesn't have to reflect our repo settings
  • now there's a new requirement (black), we have a requirements.txt file and new hatch environment for running examples
  • pages reorganised into one python file for each chart group
  • page paths are now hierarchical, e.g. http://localhost:8050/deviation/butterfly - I think it works really nicely for this example so it seemed like a good showcase for the feature which we don't demonstrate anywhere else and probably most people aren't aware is possible
  • utils generally moved/removed with just a few bits left in _pages_utils.py
  • svg files de-duplicated and no need for remove_prefix any more - the page titles map onto svgs 1-1

In terms of the actual content, I changed very little - mainly just tiny English corrections and bits. The biggest changes are:

  • remove scatter factory since it was only used once
  • copied the intro text from README.md onto a card for the All tab on homepaeg
  • decided that ordered bar/column should be distinct from bar/column so split them off (but content is still identical)
  • set opacity to 0.3 rather than 0.5 for incomplete pages since the contrast seemed a bit low to me (I love love love this way of showing something as inactive btw, super simple and effective)

Things you might still like to change in this PR. Feel free to ignore if you disagree or save it for another time if you agree but don't want to do it now, because I know this has already been a huge task:

  • is it worth putting in a reference to the data viz book that the FT based their work on also? (I'm curious, what is the source?)
  • should the code snippet occupy the same height as the graph? It feels a bit squashed to me currently?
  • the html.H3(self.title) in the CodeClipboard feels a bit redundant/clutter to me given it says "Show code" right above it, and the copy button doesn't actually apply to that but appears in a position that makes it look like it does
  • maybe nice to have an overall dashboard title? "Vizro chart gallery" or similar?
  • some of the icons used in NavLink could maybe be improved. Distribution being waterfall icon is a bit confusing given that waterfall isn't one of those charts. But I can't find any other icon that would be more suitable there :/ And there's nothing wrong with these how they currently are, but in case you prefer them: deviation to one of these? Flow to air?
  • explanation for treemap doesn't make sense where it appears on the Magnitude category, it's really written for the Part-to-whole page
  • violin plot explanation mentions quartiles but these aren't actually shown without hovering over the plot. Maybe change the explanation or use box=True or something to show them in the plot?
  • sankey plot renders with C1 overlapping other bars which looks a bit odd. You can drag and drop the bars to separate them so it's not a problem with the data, but maybe there's some option that would separate these out?
  • might be worth tweaking the black settings in black.Mode e.g. for line length - I didn't experiment with these at all, just left them on default
  • is there a simpler dataset that would work for the bar/column charts? Surely there's something in px.data that doesn't require us to do the aggregations on it?
  • did you see these look like svgs with transparent backgrounds so might work better than the black ones? I don't mind the current ones though
  • thanks for doing the change to navicon highlighting, I wasn't actually expecting you to do that! Does it mean we need to update lots of screenshots for the tests? If so then I think not worth doing here, let's just leave it as is because I also had some comments/questions on the right design for this anyway.

Aaaand finally some TODOs that we shouldn't do now but I didn't want to lose as future ideas. Maybe some could be done in Grace Hopper, maybe some before that, maybe some after that or never... These are all in the code but let me put them all here, roughly in order of lowest to highest priority:

# TODO: this is currently identical to ordered column/bar. It should be:
#  - unordered (currently ordering is done in tips_agg)
#  - different svg
#  - slightly different text
#  - slightly different example

# TODO: this is currently identical to bar/column. It should be:
#  - slightly different text since it says "you can arrange your bars in any order"

# TODO: think about the best way to do code examples, e.g.
# - do we want full dashboard example or plot-only example?
# - or both? Could be done using a toggle switch or multiple tabs.
# - a link to py.cafe showing the dashboard code?

# TODO: Charts that are commented out in incomplete_pages below do not have an svg made yet.
#  Uncomment them once they are ready.

# TODO: consider whether each chart group should have its own individual homepage, e.g. at http://localhost:8050/deviation/.
# This could just repeat the content of the tab from the homepage and would work nicely with the hierarchical navigation.

# TODO: consider how this should be represented in the code example files. Since the code is copy and pasted
# it can get out of sync. But probably we don't want the docstrings in the short code snippet.
# Ultimately these charts will probably move to vizro.charts anyway.

# TODO: eventually deduplicate page generation into a function rather than copying and pasting across files?

antonymilne and others added 7 commits July 25, 2024 23:04
- Add dashboard title
- Add reference to original data viz book
- Remove header from CodeClipboard
- Improve navigation icons
- Move navigation coloring to custom CSS
- Remove treemap from magnitude category
- Simplify sankey data
- Optimise black formatting
- Optimise layout
- Comment out navigation icon highlighting
- Add box to violing chart
@huong-li-nguyen
Copy link
Contributor Author

For my own tracking:

Things you might still like to change in this PR. Feel free to ignore if you disagree or save it for another time if you agree but don't want to do it now, because I know this has already been a huge task:

  • is it worth putting in a reference to the data viz book that the FT based their work on also? (I'm curious, what is the source?)
  • should the code snippet occupy the same height as the graph? It feels a bit squashed to me currently?
    I've played around with that quite a lot, it currently has a max. height set, because otherwise it makes the entire page scroll which didn't look nice. So the max. height it can have is the chart height on the left, and if it goes beyond that, one has to scroll the code clipboard container. What did you have in mind?
  • the html.H3(self.title) in the CodeClipboard feels a bit redundant/clutter to me given it says "Show code" right above it, and the copy button doesn't actually apply to that but appears in a position that makes it look like it does
  • maybe nice to have an overall dashboard title? "Vizro chart gallery" or similar?
  • some of the icons used in NavLink could maybe be improved. Distribution being waterfall icon is a bit confusing given that waterfall isn't one of those charts. But I can't find any other icon that would be more suitable there :/ And there's nothing wrong with these how they currently are, but in case you prefer them: deviation to one of these? Flow to air?
  • explanation for treemap doesn't make sense where it appears on the Magnitude category, it's really written for the Part-to-whole page
    I've just removed the treemap as a chart for magnitude which is also consistent with the FT. Maybe we should just deviate to bar charts and shouldn't encourage people to use treemaps here anyway 😄
  • violin plot explanation mentions quartiles but these aren't actually shown without hovering over the plot. Maybe change the explanation or use box=True or something to show them in the plot?
  • sankey plot renders with C1 overlapping other bars which looks a bit odd. You can drag and drop the bars to separate them so it's not a problem with the data, but maybe there's some option that would separate these out?
    I've noticed that as well, but unfortunately that's a common sankey issue 😬 I couldn't find a quick solution on the forums either, except for providing more space or manually calculating the node positions. For simplicity I just changed the data to the plotly docs example 😅 .
  • might be worth tweaking the black settings in black.Mode e.g. for line length - I didn't experiment with these at all, just left them on default
    I've set it to a line length of 80.
  • is there a simpler dataset that would work for the bar/column charts? Surely there's something in px.data that doesn't require us to do the aggregations on it?
    I believe it's important to demonstrate the aggregation. There isn't a simpler dataset in px.data, because as soon as you have more than one observation, aggregation becomes necessary; otherwise, the chart appears as shown below, which can be quite confusing. What Plotly does is stack each observation. The simplest dataset would be one we create from scratch with a single observation. However, I wanted to show the aggregation because I remember how confusing it was initially to see charts like this, as it's not what you would expect. To get a clean chart, you need to add an aggregation in about 80% of the cases 😄
    Image
  • did you see these look like svgs with transparent backgrounds so might work better than the black ones? I don't mind the current ones though
    You can change all our SVGs to have transparent backgrounds, which isn't an issue. However, I think the dark theme looks less appealing with transparent backgrounds. To ensure the theme switch works well without compromising the appearance, you would need to make all SVGs transparent and create a custom card with an additional container that has the correct background color. I've decided that it's not worth the hassle at the moment, and I actually kind of like the dark-on-light theme 😄
  • thanks for doing the change to navicon highlighting, I wasn't actually expecting you to do that! Does it mean we need to update lots of screenshots for the tests? If so then I think not worth doing here, let's just leave it as is because I also had some comments/questions on the right design for this anyway.
    I can remove it or we just leave it as custom.css for this demo dashboard. In that case we don't have to update the screenshots and I kind of like it actually 😄

@huong-li-nguyen
Copy link
Contributor Author

huong-li-nguyen commented Jul 29, 2024

  • # TODO: this is currently identical to ordered column/bar. It should be:
    • unordered (currently ordering is done in tips_agg)
    • different svg
    • slightly different text
    • slightly different example
  • # TODO: this is currently identical to bar/column. It should be: slightly different text since it says "you can arrange your bars in any order"
  • # TODO: think about the best way to do code examples, e.g.
    • do we want full dashboard example or plot-only example?
    • or both? Could be done using a toggle switch or multiple tabs.
    • a link to py.cafe showing the dashboard code?
  • # TODO: Charts that are commented out in incomplete_pages below do not have an svg made yet. Uncomment them once they are ready.
  • # TODO: consider whether each chart group should have its own individual homepage, e.g. at http://localhost:8050/deviation/. This could just repeat the content of the tab from the homepage and would work nicely with the hierarchical navigation.
    I think that's what the overall homepage is for. I would rather want them to go back there and use the tabs instead of creating a duplicating homepage for each subgroup and adding another way of navigation for the user. Would keep it simple
  • # TODO: consider how this should be represented in the code example files. Since the code is copy and pasted it can get out of sync. But probably we don't want the docstrings in the short code snippet.
    Ultimately these charts will probably move to vizro.charts anyway.
  • # TODO: eventually deduplicate page generation into a function rather than copying and pasting across files?

@huong-li-nguyen huong-li-nguyen enabled auto-merge (squash) July 29, 2024 16:12
@huong-li-nguyen huong-li-nguyen disabled auto-merge July 29, 2024 16:16
@huong-li-nguyen huong-li-nguyen enabled auto-merge (squash) July 29, 2024 16:17
@huong-li-nguyen huong-li-nguyen merged commit cd76b81 into main Jul 29, 2024
30 checks passed
@huong-li-nguyen huong-li-nguyen deleted the demo/chart-gallery branch July 29, 2024 16:21
nadijagraca pushed a commit that referenced this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants