-
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 creation UI #160179
[Dashboard Navigation] Add creation UI #160179
Conversation
b4d1adb
to
b1d9f30
Compare
8863ace
to
7a0ac38
Compare
2e53f49
to
6faccad
Compare
87aa84a
to
2198908
Compare
4db8743
to
bc9c863
Compare
0863f99
to
5768166
Compare
7165859
to
e70ed3c
Compare
19e02b7
to
2b82446
Compare
7aaa593
to
7d1864b
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
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! I added a couple of nits, but nothing that needs to block merging.
})} | ||
<EuiButtonEmpty | ||
size="s" | ||
flush="left" |
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.
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.
Ooooh, good catch - I definitely agree that looks pretty ugly 👀
I'm a little bit worried about how that link will look without flush="left"
once we add the "Save to library" toggle from the designs:
But that's something we can address as part of that PR :) I think removing the left flush is the right call here.
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.
Fixed in c0c81e2 👍
Without focus | With focus |
---|---|
{!links || Object.keys(links).length === 0 ? ( | ||
<EuiPanel hasBorder={true}> | ||
<EuiFlexGroup justifyContent="spaceAround"> | ||
<EuiFlexItem grow={false}> | ||
<EuiText size="s"> | ||
{NavEmbeddableStrings.editor.panelEditor.getEmptyLinksMessage()} | ||
</EuiText> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
<EuiSpacer size="s" /> | ||
<EuiFlexGroup justifyContent="spaceAround"> | ||
<EuiFlexItem grow={false}> | ||
<EuiButton | ||
onClick={() => setShowLinkEditorFlyout(true)} | ||
iconType="plusInCircle" | ||
> | ||
{NavEmbeddableStrings.editor.getAddButtonLabel()} | ||
</EuiButton> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
</EuiPanel> | ||
) : ( |
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.
Doesn't have to block this PR, but I wonder if we should (can?) add a nice empty prompt to the flyout? It would be nice to have a title prompt like "Create links" and a description explaining the value of the links panel, e.g. "Links can pass filters and time range to other dashboards. Or create external links to other websites." <-- probably needs some better copy 😄
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.
I 100% agree - @andreadelrio @gchaps any ideas on how to add some visual interest to the flyout and/or improve the copy to give it more impact when there are no links? 👀 For reference, this is what the flyout currently looks like when first creating a links panel:
I definitely think there's work we could do to make it a little more ⭐ fancy ⭐ even if it's not included as part of this PR :)
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.
@Heenawter we can definitely improve the text. I'll post a suggestion.
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.
@gchaps Yes, it is! 👍 It is still in its early stages, but we are essentially creating a navigation system so that users don't have to use markdown links (which can very easily become out of date, especially when using hard links to specific dashboards) anymore. I can link you to the project docs on Slack if you want more information - sorry for the sudden no-context mention, haha! 😅
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.
I agree that we could make this more visually appealing. Let me think about what we can do here. I don't think we have any illustration available right now that would fit the bill so there might not be a quick solution to this.
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.
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.
Wow! The first one with the big link and the fly looks incredible!
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.
I'm partial to the first graphic.
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.
Oooh, those are fantastic Andrea 🔥 I especially like the first one!! :) I've opened a related issue so that we can move discussion of the empty state there (both for copy suggestions and the graphic) - I think it's worth getting multiple opinions, and this thread is already getting pretty large hehe
2736613
to
d71df9e
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.
@Heenawter Great job on this first PR!
Here are some things to consider design-wise:
- I think we need to go full width with the inputs in the flyout.
- I'm also wondering whether we should have the
Current
badge absolutely positioned. Right now, in the event of a super long dashboard name, the badge won't be visible. - The
Current
badge should not be getting an underline on hover. - For the
Text
input I think we should either:
a) automatically prefill it with the text of the chosen destination.
b) mark it as optional - For the
Choose destination
input, I suggest we show the error text below it.
Done :)
Very good point! We could prepend or append it to the
I think I prefer prepending, since it looks better when the current dashboard name isn't long:
What do you think? 👀
Good catch - that will be fixed by using either the
How do we usually mark something as optional in a form? I looked through Kibana + the EUI form documentation and I couldn't find any examples - totally possible I missed something, though. Would something as simple as this work? Update: Actually, at some point I had the placeholder getting replaced... That would probably help a lot, I don't know when that broke 😆 Let me try to fix that...
Sorry, which error text are you referring to? There's no error text at the moment - do you mean once we add better validation to the URL input as discussed here? |
I like the prepend better precisely for the reason you mentioned.
Yes, that's perfect. According to EUI's guidelines, all fields are considered required unless marked as optional (using either the label of the help text)
Yes, I was referring to that 👍 |
Let's go with that, then 🎉
Done! :) I also fixed the issue with the placeholder text not being replaced: Screen.Recording.2023-07-07.at.4.21.19.PM.mov(ignore the
Awesome, yeah - that makes sense to me 👍 |
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.
Added a couple more nits. Should be easy changes.
src/plugins/navigation_embeddable/public/components/navigation_embeddable_link_editor.tsx
Outdated
Show resolved
Hide resolved
src/plugins/navigation_embeddable/public/components/navigation_embeddable_link_editor.tsx
Show resolved
Hide resolved
💔 Build FailedFailed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Closes #154360
Summary
This PR adds the first draft of the UI for creating the navigation embeddable and adding links to it. Note that this PR only addresses adding links - you cannot currently remove links from the panel or edit existing links. Also, because this PR contains critical code, some cleanup tasks were left as
TODO
in order to get it merged ASAP - these will be addressed in follow up PRs.Screen.Recording.2023-07-06.at.4.31.04.PM.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