feat(explore-popover): Show disabled 'Save' button in explore popover #21318
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21318 +/- ##
==========================================
+ Coverage 65.38% 66.88% +1.50%
==========================================
Files 1847 1805 -42
Lines 70561 69034 -1527
Branches 7737 7368 -369
==========================================
+ Hits 46137 46175 +38
+ Misses 22417 20953 -1464
+ Partials 2007 1906 -101
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
/testenv up |
|
@geido Ephemeral environment spinning up at http://54.202.131.27:8080. Credentials are |
|
Hello @agl-developer a few things. In other controls when the form is ready to be saved the buttons looks like the one in the screenshot. I think we shouldn't introduce inconsistencies unless we are going to change the behaviour for every control. Personally, I think what's shown in the screenshot is a clearer CTA.
One more thing, when editing the SQL the control correctly allows me to save. However, if I revert the changes to the SQL statement to its original state before saving, it will still allow me to save even though there was no actual change compared to the previous state |
| @@ -118,26 +118,14 @@ test('Clicking on "Close" should call onClose', () => { | |||
| expect(props.onClose).toBeCalledTimes(1); | |||
| }); | |||
|
|
|||
| test('Clicking on "Save" should call onChange and onClose', () => { | |||
| test('Clicking on "Save" should not call onChange and onClose', () => { | |||
There was a problem hiding this comment.
I believe we need to separate the tests here for both calling and not calling
| wrapper | ||
| .instance() | ||
| .onAdhocFilterChange(simpleAdhocFilter.duplicateWith({ operator: null })); | ||
| expect(wrapper.find(Button).find({ disabled: true })).toExist(); | ||
| expect(wrapper.find(Button).find({ disabled: false })).toExist(); |
There was a problem hiding this comment.
Shouldn't the button be disabled at this point?
| buttonStyle={ | ||
| hasUnsavedChanges && stateIsValid ? 'primary' : 'default' | ||
| } | ||
| disabled={stateIsValid && !hasUnsavedChanges} |
There was a problem hiding this comment.
Shouldn't we account for an invalid state as well?
|
/testenv up |
|
@geido Ephemeral environment spinning up at http://35.93.39.51:8080. Credentials are |
|
On top of the comments above to which I am still looking for an answer, I think there is another problem. When adding a new control the button shows enabled even though I haven't filled any of the required fields.
|
… when no changes have been made
michael-s-molina
force-pushed
the
feat-53936
branch
from
02b2668
to
2962bde
Compare
|
/testenv up |
|
@geido Ephemeral environment spinning up at http://54.245.47.220:8080. Credentials are |
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
In the popover that we show in control panel, if there are no changes made we turn it into secondary button and user is still able to "save" the popover.
If there are no changes made in the popover use primary button in state "disabled"
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION