-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Make StatusWidget tools dropdown extensible #3189
Make StatusWidget tools dropdown extensible #3189
Conversation
9b47c90
to
d217796
Compare
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.
Thank you for the PR! Just some minor comments from me.
I know @askvortsov1 doesn't want many changes to the Admin dashboard at the moment, since it'll be getting a rewrite in the near future, so I'd appreciate his input.
Personally, I think we can make changes up until v1.2. That way, when rewriting, we can use the existing dashboard as a benchmark to ensure features or extensibility aren't accidentally dropped between the two, and to make the overall migration easier.
Also it looks like you've rebased this incorrectly, as there are some changes to the file versioner since I began writing my review. Could you make sure those are fixed, too? |
d217796
to
7bab6ed
Compare
@davwheat Uhhh, managed to fix my commits. Sorry for that 🤦♂️ |
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.
We might change things eventually, but for now I'm glad for this to be more extensible.
Changes proposed in this pull request:
This PR allows you to more easily modify the tools dropdown in the
StatusWidget
component usingItemList
.Until now, if you wanted to add something to this dropdown, you had to push a child:
If you want to remove / replace a given item it gets harder.
Now such code is enough:
Reviewers should focus on:
Confirm that everything works.
Screenshot
Necessity
Confirmed