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

Improvement of custom charts docs #229

Merged
merged 15 commits into from
Dec 21, 2023
Merged

Conversation

maxschulz-COL
Copy link
Contributor

@maxschulz-COL maxschulz-COL commented Dec 19, 2023

Description

  • short PR to improve docs on custom charts - increased emphasis that they can be used with FIlter and Parameter
  • especially the emphasis on Parameter hopefully guides people towards the great flexibility that this offers

Now added:

  • change the docs on custom components to have a shorter example - they now make use of super instead of copying the entire build method

Screenshot

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.

@maxschulz-COL maxschulz-COL added Status: Ready for Review ☑️ Issue/PR is ready for review - all tests have passed Docs 🗒️ Issue/PR for markdown and API documentation labels Dec 19, 2023
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

Thanks for addressing this so immediately. 🚀 The flexibility of the custom charts feature is much cleaner now.


Do you think we should change the order of custom charts with Filters and Parameters and list them in the order:

  • custom chart with Filter and then
  • custom chart with Parameter

Also, do you think we should add following comments below def scatter_with_line(...) and def waterfall(...):

  • # "data_frame" is filtered.
  • # "hline" is parametrised.
    or something like that?

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I have one favor to ask given that this fits into this PR quite well - there is an item on our list to update the docs for the custom components as well. @antonymilne can give you more details, but in general it's also just updating the example and adding 1-2 sentences.

In the current docs, the changes for the custom slider look quite complex, while it can actually be achieved quicker and easier - see @petar-qb answer on the user question here: #187

Let me know if that's fine for you, otherwise I can also create a separate PR 👍

vizro-core/docs/pages/user_guides/custom_charts.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/custom_charts.md Outdated Show resolved Hide resolved
@maxschulz-COL maxschulz-COL changed the title Docs/improve custom chart docs Improvement of custom charts docs Dec 20, 2023
@maxschulz-COL
Copy link
Contributor Author

Thanks for addressing this so immediately. 🚀 The flexibility of the custom charts feature is much cleaner now.

Do you think we should change the order of custom charts with Filters and Parameters and list them in the order:

  • custom chart with Filter and then
  • custom chart with Parameter

Also, do you think we should add following comments below def scatter_with_line(...) and def waterfall(...):

  • "data_frame" is filtered.

  • "hline" is parametrised.

    or something like that?

I implemented the above suggestions as clickable code annotations. Also improved the title of the examples, but I did not swap the order. Actually I prefer having the parameter on top, as its more important to get across.

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Have minor comments on rephrasings - feel free to add it or not :)

vizro-core/docs/pages/user_guides/custom_charts.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/custom_components.md Outdated Show resolved Hide resolved
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.

Thanks for putting the extra custom component change in there too!

Just while I remember, @Coding-with-Adam had some feedback that as a new user it was a little difficult to understand from the current docs the difference between filter and parameter and action. Probably he has lots of other valuable feedback too so we should quiz him in the new year 🙂

vizro-core/docs/pages/user_guides/custom_components.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/custom_components.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/custom_components.md Outdated Show resolved Hide resolved
maxschulz-COL and others added 4 commits December 21, 2023 09:57
Co-authored-by: Antony Milne <49395058+antonymilne@users.noreply.github.com>
Signed-off-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
Co-authored-by: Antony Milne <49395058+antonymilne@users.noreply.github.com>
Signed-off-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
Co-authored-by: Antony Milne <49395058+antonymilne@users.noreply.github.com>
Signed-off-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
@maxschulz-COL maxschulz-COL merged commit ec4f6fb into main Dec 21, 2023
33 checks passed
@maxschulz-COL maxschulz-COL deleted the docs/improve_custom_chart_docs branch December 21, 2023 13:37
@Coding-with-Adam
Copy link

Probably he has lots of other valuable feedback too so we should quiz him in the new year 🙂

Happy to help. adam@plot.ly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs 🗒️ Issue/PR for markdown and API documentation Status: Ready for Review ☑️ Issue/PR is ready for review - all tests have passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants