-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
PanelEdit: UX improvements #32124
PanelEdit: UX improvements #32124
Conversation
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.
💯🔥🔥
just a few typescript/style nitpicks. I think it would be really great to not introduce any more any
type holes (or, while we're at it, cover up the existing ones) - just my two cents.
onPanelConfigChange: (configKey: string, value: any) => void; | ||
onPanelOptionsChanged: (options: any) => void; |
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.
Do you think it's possible to type options
and value
? IMHO any
should be a lint error (but that's a conversation for another day...)
I haven't exactly been able to glean from this what it's supposed to be (which is a part of the problem), but maybe something like PanelOptions = Record<string, string | number | PanelOptions>
would be a lot more helpful?
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.
@torkelo maybe unknown
might be better then? Treat it as an opaque type that needs refining before it can be used, to reduce the temptation to make unsafe assumptions about it.
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.
could type as as Record<string, any>
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.
Record<string, unknown>
😌
just trying to think about how we can enforce preventing unsafe assumptions about the code that leads to runtime errors.
const [showDeletionModal, setShowDeletionModal] = useState(false); | ||
|
||
const onDeletePanel = () => { | ||
onDelete?.(libraryPanel); | ||
setShowDeletionModal(false); | ||
}; | ||
|
||
const panelPlugin = config.panels[libraryPanel.model.type] ?? ({} as PanelPluginMeta); |
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.
This {} as PanelPluginMeta
doesn't look like a safe to me. PanelPluginMeta
has nested properties which looks like are later accessed in PanelTypeCard
which would throw a runtime error.
In what scenario is panelPlugin
undefined? Maybe PanelTypeCard
should just be not rendered at all if it's not there?
(tbh this should also be a lint error, or at least a warning. but seperate conversation)
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.
yea think if the panel plugin cannot be found here we need some error handling instead
.addItem( | ||
new OptionsPaneItemDescriptor({ | ||
title: 'Max per row', | ||
showIf: () => (panel.repeat && panel.repeatDirection === 'h') as boolean, |
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 know this is a relatively safe, but maybe it might just be better to use !!(...)
to actually ensure it's actually a boolean rather than to just use a type cast
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 don't agree, you should always use Boolean(...)
instead of !!
. So much easier to read.
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.
Sure. I don't share the concern about !!, but Boolean() is also great. As long as we're not telling Typescript to lie to us 😅
public/app/features/dashboard/components/PanelEditor/AngularPanelOptions.tsx
Outdated
Show resolved
Hide resolved
public/app/features/dashboard/components/PanelEditor/OverrideEditor.tsx
Outdated
Show resolved
Hide resolved
public/app/features/dashboard/components/PanelEditor/OptionsPaneCategory.tsx
Outdated
Show resolved
Hide resolved
public/app/features/dashboard/components/PanelEditor/OptionsPaneOptions.tsx
Show resolved
Hide resolved
public/app/features/dashboard/components/PanelEditor/getFieldOverrideElements.tsx
Outdated
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.
@ryantxu ah, good catch. Library panels is still behind feature toggle, and if not enabled it will 404 |
* origin/master: (38 commits) GraphNG: Fix tooltip displaying wrong or no data (grafana#32312) Render new email template and fix the title (grafana#32314) revert changes (grafana#32335) Add documentation page for how to install Grafana and GE on K8s (grafana#32008) Chore: use uppercase names in manual entry scenario (grafana#32334) live: fix writing headers in hijacked connection with gzip enabled (grafana#32310) ReleaseNotes: Updated changelog and release notes for 7.5.0 (grafana#32318) Release: Updates latest.json (grafana#32319) GraphNG: updates story from knobs to controls (grafana#32294) Library Panels: Improve unsaved changes handling (grafana#32026) Upgrade Prometheus Alertmanager and small fixes (grafana#32280) Chore: eslint react hook fix for public folder (grafana#31174) DashboardSettings: Migrates history diff view from angular to react (grafana#31997) chore(deps): bump visx/tooltip to 1.7.2 (grafana#32248) Loki: Fix text search in Label browser (grafana#32293) Docs: Adjust sizes of images in Loki documentation (grafana#32301) PanelEdit: v8 Panel Edit UX (grafana#32124) Timeseries: add UI to control the span nulls threshold (grafana#32222) Alerting: add state tracker to alerting evaluation (grafana#32298) Alerting: Add StartAt and FiredAt to the alert evaluation result (grafana#32302) ...
* Initial commit * Progress * Update * Progress * updates * Minor fix * fixed ts issue * fixed e2e tests * More explorations * Making progress * Panel options and field options unified * With nested categories * Starting to find something * fix paddings * Progress * Breakthrough ux layout * Progress * Updates * New way of composing options with search * added regex search * Refactoring to react note tree * Show overrides * Adding overrides radio button support * Added popular view * Separate stat/gauge/bargauge options into value options and display options * Initial work on getting library panels into viz picker flow * Fixed issues switching to panel library panel * Move search input put of LibraryPanelsView * Changing design again to have content inside boxes * Style updates * Refactoring to fix scroll issue * Option category naming * Fixed FilterInput issue * Updated snapshots * Fix padding * Updated viz picker design * Unify library panel an viz picker card * Updated card with delete action * Major refactoring back to an object model instead of searching and filtering react node tree * More refactoring * Show option category in label when searching * Nice logic for categories rendering when searching or when only child * Make getSuggestions more lazy for DataLinksEditor * Add missing repeat options and handle conditional options * Prepping options category to be more flexibly and control state from outside * Added option count to search result * Minor style tweak * Added button to close viz picker * Rewrote overrides to enable searching overrides * New search engine and tests * Searching overrides works * Hide radio buttons while searching * Added angular options back * Added memoize for all options so they are not rebuilt for every search key stroke * Added back support for category counters * Started unit test work * Refactoring and base popular options list * Initial update to e2e test, more coming to add e2e test for search features * Minor fix * Review updates * Fixing category open states * Unit test progress * Do not show visualization list mode radio button if library panels is not enabled * Use boolean * More unit tests * Increase library panels per page count and give search focus when switching list mode * field config change test and search test * Feedback updates * Minor tweaks * Minor refactorings * More minimal override collapse state
* Initial commit * Progress * Update * Progress * updates * Minor fix * fixed ts issue * fixed e2e tests * More explorations * Making progress * Panel options and field options unified * With nested categories * Starting to find something * fix paddings * Progress * Breakthrough ux layout * Progress * Updates * New way of composing options with search * added regex search * Refactoring to react note tree * Show overrides * Adding overrides radio button support * Added popular view * Separate stat/gauge/bargauge options into value options and display options * Initial work on getting library panels into viz picker flow * Fixed issues switching to panel library panel * Move search input put of LibraryPanelsView * Changing design again to have content inside boxes * Style updates * Refactoring to fix scroll issue * Option category naming * Fixed FilterInput issue * Updated snapshots * Fix padding * Updated viz picker design * Unify library panel an viz picker card * Updated card with delete action * Major refactoring back to an object model instead of searching and filtering react node tree * More refactoring * Show option category in label when searching * Nice logic for categories rendering when searching or when only child * Make getSuggestions more lazy for DataLinksEditor * Add missing repeat options and handle conditional options * Prepping options category to be more flexibly and control state from outside * Added option count to search result * Minor style tweak * Added button to close viz picker * Rewrote overrides to enable searching overrides * New search engine and tests * Searching overrides works * Hide radio buttons while searching * Added angular options back * Added memoize for all options so they are not rebuilt for every search key stroke * Added back support for category counters * Started unit test work * Refactoring and base popular options list * Initial update to e2e test, more coming to add e2e test for search features * Minor fix * Review updates * Fixing category open states * Unit test progress * Do not show visualization list mode radio button if library panels is not enabled * Use boolean * More unit tests * Increase library panels per page count and give search focus when switching list mode * field config change test and search test * Feedback updates * Minor tweaks * Minor refactorings * More minimal override collapse state
Library panels
Support search this changes the structure for how options are rendered, instead of rendering it directly in different components the code first fetches all options and groups together with metadata (property name) and value. So we have it in a structure that we can search and filter.
WIP state: (Notice how table Show header option is next to field options)


Tried a million alternatives with more nested layouts, some can be found here:
https://www.figma.com/file/7tTwQzazE1QmLFaA5vHujy/Panel-edit-tweaks-Grafana-8?node-id=99%3A0