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

Update _pages.py - fix sorting for > 10 pages #2565

Merged
merged 2 commits into from
Jun 16, 2023
Merged

Update _pages.py - fix sorting for > 10 pages #2565

merged 2 commits into from
Jun 16, 2023

Conversation

gothicVI
Copy link
Contributor

Fix #2564

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • task 1
    • task 2
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@gothicVI gothicVI requested a review from alexcjohnson as a code owner June 14, 2023 10:46
dash/_pages.py Outdated Show resolved Hide resolved
@gothicVI
Copy link
Contributor Author

@alexcjohnson @AnnMarieW
I've included the proposed change and added some test pages for

  • >10 pages
  • fractional ordering
  • no order given

I hope these cover the changes well.

dash/_pages.py Outdated Show resolved Hide resolved
dash/_pages.py Outdated Show resolved Hide resolved
@AnnMarieW
Copy link
Collaborator

@gothicVI Thanks for posting about this issue on the forum and creating this pull request. It looks great!

@alexcjohnson regarding the question about id in the page_registry:

The id is not used in this test, but it is used in others in this folder.

Typically when assigning an id to components like buttons and links by looping through the dash.page_registry I like to use an id prop rather than using module. I think it makes the code more readable, and also if you change a file name in the pages folder it won’t break callbacks.

However, for tests, I think it would have been simpler - and better to just use the module. I could make a separate PR to refactor this if you like.

@alexcjohnson
Copy link
Collaborator

Thanks for the explanation of id @AnnMarieW - not a big deal, you're welcome to change it but it's not hurting anything.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Thanks very much @gothicVI!

@alexcjohnson alexcjohnson merged commit ebc6988 into plotly:dev Jun 16, 2023
@gothicVI gothicVI deleted the patch-1 branch June 16, 2023 14:49
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.

[BUG] wrong sorting of registered pages by ordering
3 participants