-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Dashboard Navigation] Add link editing + reordering #161568
[Dashboard Navigation] Add link editing + reordering #161568
Conversation
40e6264
to
98e05fb
Compare
9784332
to
2ddb6ec
Compare
56a6cf4
to
1973cae
Compare
cc7f0d4
to
9ade9ab
Compare
@Heenawter I think I was thinking of the |
@andreadelrio Ahhh, that's an excellent point... I definitely think it could be valuable to show the selected dashboard title somewhere, although I'm not exactly sure where. We could do something with an ... or something like the data view picker in the controls creation flyout with a popover + ... but I'm not convinced that those make the best use of the real estate we have in the flyout - it would probably look pretty empty 😅 For the field picker in the controls flyout, it looks like we only show the name of the selected field in the We could have a similar idea to the Not sure if that's overkill though? |
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 could have a similar idea to the Control type row, which auto-populates with the selected dashboard/URL....
Jul-14-2023 09-36-37
Not sure if that's overkill though?
Of the proposed options, I like this idea the best.
I do sometimes forget that the EuiSelectable
here is only for single selections. My muscle memory expects to use CMD-click to select multiple dashboards. That would be a slight concern for me if I was a user wanting to quickly create a very long sidebar list of links. But then users would also not be able to change the label on a link during creation.
But we can always add multi-select in a later release. Not necessary for MVP, IMO.
src/plugins/navigation_embeddable/public/components/navigation_embeddable_panel_editor.tsx
Outdated
Show resolved
Hide resolved
341153d
to
be256b1
Compare
Major design changeAfter discussion with @andreadelrio, we have decided to go with an Screen.Recording.2023-07-14.at.4.20.37.PM.movThis was implement in bdf7756. |
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.
lgtm! EuiComboBox is a great improvement!
code review and tested add/edit link functionality
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.
LGTM! EuiComboBox feels a lot cleaner, thanks for implementing it. We might want to think about doing the same for Controls in the future.
💔 Build FailedFailed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @Heenawter |
Closes #154361
Closes #161274
Closes #161693
Summary
This PR adds editing capabilities to the navigation embeddable, including deleting/editing existing links and reordering the list of links. It also fixes the delay in opening the editing flyout from the async import in
getExplicitInput
(from the navigation embeddable factory) by moving it to the constructor of the factory.Screen.Recording.2023-07-12.at.11.25.46.AM.mov
Checklist
Unit or functional tests were updated or added to match the most common scenariosWill be addressed in [Dashboard Navigation] Add functional + unit tests #161287For maintainers