-
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
ability to set human readable title of data view & ability to edit data view #124191
Conversation
|
src/plugins/data_view_editor/public/components/form_fields/title_field.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data_view_editor/public/components/preview_panel/status_message/status_message.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/components/sidebar/discover_index_pattern.tsx
Outdated
Show resolved
Hide resolved
This is a great enhancement. I just have some comments/questions:
Data view picker
|
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.
A few thoughts:
Edit Data view flyout
Shouldn't the index pattern be labeled as 'Index pattern' instead of Data view?
The footer button action should be 'Save' instead of 'Edit data view'
Manage data view
We should show both the title and index pattern here.
Actions dropdown:
Since we only have the two options (for now) and the add field button is still by the table, let's just make these two options exposed. Using the action menu was in conjuction with the other page edits
Data view selection
I don't think we need the popover title here.
And I think we can rename the two options to:
Add field to this data view
Manage this data view
And change the search placeholder to
Find a data view
And the create button at the bottom to simply
New data view
Are we still wanting to provide a title by default (reusing the index pattern as an initial value). It seems this list will reference the title if available and fallback to the index pattern.
This might be confusing.
The concern is the following example where one is showing the title of a data view, while the others are showing the index pattern
Some of these edits get to question of how much we want to push data view titles at the onset. So perhaps I'm jumping ahead a bit, but it would seem we are treating these as simply optional descriptions at the moment rather than a title (which might be better anyway).
Either way, we should be consistent in how we're displaying these. If we just want these as optional descriptions, then let's just always show the index pattern in the dropdowns from Discover, Lens, etc. Then also keep the index pattern as h1 on the manage data view screen (with the optional title below it). But I'm a bit concerned swapping these depending on whether the title is filled out. We should always be referencing these as either the title or the index pattern in my opinion.
x-pack/plugins/lens/public/indexpattern_datasource/layerpanel.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx
Outdated
Show resolved
Hide resolved
I'm reviewing the mockups and making sure that everything in them makes it into a PR. I think it would be reasonable for this PR to include -
from #124346 |
@mattkime Thanks for double-checking. One question I had was whether we were always showing either the title or the index pattern in the dropdown for Discover, Lens, etc. I think we should be consistent so some aren't referencing the data view title and others the index pattern. |
@mdefazio I think data view management is a special case. Showing the index patterns in the list form could be very helpful. As for providing the index pattern in dropdowns - I guess I'd shy away from that. If users want the index pattern to be part of the name they can always make it part of the name. |
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.
Yay! thanx for addressing my comments! LGTM!
src/plugins/discover/public/application/main/components/top_nav/discover_topnav.tsx
Show resolved
Hide resolved
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
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.
Code LGTM, didn't test locally. Had one request for a code comment.
src/plugins/management/public/components/management_app/management_app.tsx
Show resolved
Hide resolved
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
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Images
Checklist
Delete any items that are not applicable to this PR.