-
Notifications
You must be signed in to change notification settings - Fork 4.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
[Widget Params] Split title and mapping editing (2 of 3) #3334
[Widget Params] Split title and mapping editing (2 of 3) #3334
Conversation
@@ -149,7 +150,7 @@ export class ParameterMappingInput extends React.Component { | |||
<div className="m-t-10"> | |||
<Select | |||
className="w-100" | |||
defaultValue={mapping.mapTo} | |||
value={mapping.mapTo} |
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 is so that if edit is cancelled (making state.mapping
reset to original props.mapping
), it would affect these input components, or else they would have stayed as left.
</div> | ||
); | ||
} | ||
|
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.
Bye bye!
return 'Static value'; | ||
default: | ||
return ''; // won't happen (typescript-ftw) | ||
} |
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.
Moved to getSourceTypeLabel
3eefd57
to
6e6b56e
Compare
@ranbena I can review this one directly instead of the previous one, right? |
Yeah, although it's still incomplete until the dependency is fixed. |
6e6b56e
to
b8779dd
Compare
ab9f2dd
to
b773c71
Compare
b8779dd
to
958ded0
Compare
b773c71
to
2f97354
Compare
@arikfr ready! |
1. I think it makes sense to use a wider Modal: Update: I actually noticed we already use a wider one when you edit parameters. 2. Instead of "Change parameter value" maybe "Set Parameter value"? 3. When adding the widget to a dashboard with a static value it complaints that no value is set for 4. (NTH) When the Popover is open, clicking on Esc closes the Modal instead of the Popover. 5. Completely unrelated but I think we should pin to top (along with the header) the parameters input. I added a new widget that takes a dashboard parameter, but the input was not even seen on screen. Feels a bit disorienting. I will open an issue for this. 6. (should be fixed but not a blocker:)
7. We use the |
My only reservation is that the above inputs (query and visualization) would be incredibly long.
This is trampled in the next commit #3377.
That's a bug on master. I'll open a separate issue. EDIT: #3379
Right. I'll have a separate PR to solve this with an Ant Modal. #3387
Definitely a bug. I'll look into it. EDIT: fixed
Definitely. Will open a separate issue. EDIT: #3380 |
958ded0
to
028278f
Compare
Moved to #3332 |
This is the second of a batch of 3 commits that change UX of widget params.
Why?
Currently, there is one edit button per parameter, opening a popover that allows editing both source type and title. A better user experience is to have edit buttons per editable content - one for title, one for source type.
How it looks
Title edit
Source type edit
Implementation notes
Since now popovers open to the right of the content, affecting data as you type makes the popover jump horizontally as the position of the edit button shifts.
So, I implemented a "cancel/save" to each popover, hence the underlying table stays put during editing.