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

[Docs] Update code snippets to remove unnecessary id assignment #863

Merged
merged 12 commits into from
Nov 18, 2024

Conversation

stichbury
Copy link
Contributor

@stichbury stichbury commented Nov 11, 2024

Description

As per #713

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.

@stichbury stichbury self-assigned this Nov 11, 2024
@stichbury stichbury added the Docs 🗒️ Issue for markdown and API documentation label Nov 11, 2024
@github-actions github-actions bot added the Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package label Nov 11, 2024
Copy link
Contributor

github-actions bot commented Nov 11, 2024

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2024-11-18 09:18:05 UTC
Commit: 15183e8

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/

@stichbury stichbury marked this pull request as ready for review November 12, 2024 10:05
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.

Thanks for doing this! ⭐

I didn't check if these are all the use cases, but hopefully these were the last ones!

I noticed in a few example, that the id needs to be removed on the yaml equivalent as well. Could you just double-check all of the examples you've changed whether they have a yaml equivalent that needs to be changed?

@petar-qb
Copy link
Contributor

Thanks for cleaning Vizro docs 🥇

I found several places where figure id is unnecessary:

File: explore-components.md:

  • Line: 110 - ID: line_gpd
  • Line: 306 - ID: box_cont
  • Line: 311 - ID: line_gpd
  • Line: 318 - ID: box_cont and line_gpd from targets.
  • Line: 419 - ID: box_cont
  • Line: 424 - ID: line_gpd
  • Line: 431 - ID: box_cont and line_gpd from targets.
  • Line: 600 - ID: box_cont
  • Line: 605 - ID: line_gpd
  • Line: 612 - ID: box_cont and line_gpd from targets.

Also from card-button.md:

  • Line: 635 -> , selector=vm.Dropdown(title="Species") part of this line can be removed.

@stichbury
Copy link
Contributor Author

stichbury commented Nov 14, 2024

Thanks @petar-qb! I haven't removed some of those in the explore-components.md because while they may not always be used in the place they're first introduced, they are used later in the tutorial as the filters etc are added. I think it's clearer for the reader to keep the code consistent and add just the filter when that section arises, rather than add the filter plus the target id into code that's already been discussed. Does that sound reasonable?

Also from card-button.md:

  • Line: 635 -> , selector=vm.Dropdown(title="Species") part of this line can be removed.

Sorry, I'm not following -- where is there an id in that line?

@stichbury
Copy link
Contributor Author

I noticed in a few example, that the id needs to be removed on the yaml equivalent as well. Could you just double-check all of the examples you've changed whether they have a yaml equivalent that needs to be changed?

Thanks for the catch. I've fixed those two, which were the only ones that had yaml equivalents.

Please can you re-review/approve when you've a moment?

@petar-qb
Copy link
Contributor

@stichbury

Does that sound reasonable?

This sounds very reasonable. Let's keep them then 😃

Sorry, I'm not following -- where is there an id in that line?

My fault.. It's not the ID issue there but the potential simplification that could be made.
This line in the docs is the same as the following one (so decide whether you want to simplify it within this PR):
controls=[vm.Filter(column="species")],

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. 👍

@stichbury
Copy link
Contributor Author

My fault.. It's not the ID issue there but the potential simplification that could be made. This line in the docs is the same as the following one (so decide whether you want to simplify it within this PR): controls=[vm.Filter(column="species")],

Ah, I see, thanks @petar-qb! I've made that change now. Please could you check this commit is OK and approve if so?

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.

This looks good! Thanks @stichbury for handling this 🎨

@stichbury stichbury enabled auto-merge (squash) November 15, 2024 09:23
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.

Lgtm if we leave the vizro-ai output in

@stichbury stichbury merged commit d989b32 into main Nov 18, 2024
37 of 38 checks passed
@stichbury stichbury deleted the docs/update-code-snippets branch November 18, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs 🗒️ Issue for markdown and API documentation Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants