-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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] new, bulk actions for delete & export #8979
Conversation
8f4c785
to
5c960ef
Compare
Codecov Report
@@ Coverage Diff @@
## master #8979 +/- ##
==========================================
+ Coverage 59.16% 59.52% +0.35%
==========================================
Files 367 369 +2
Lines 11679 11738 +59
Branches 2862 2880 +18
==========================================
+ Hits 6910 6987 +77
+ Misses 4590 4572 -18
Partials 179 179
Continue to review full report at Codecov.
|
describe('ConfirmStatusChange', () => { | ||
const action = jest.fn(); | ||
const mockedProps = { | ||
title: 'please confirm', |
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.
t('please confirm')
?
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 just a test, so raw string should be ok.
71f7165
to
2d58aa6
Compare
2d58aa6
to
2c1a9fd
Compare
const defaultCallback = () => { console.error('ConfirmStatusChange invoked with the default callback, please provide a function to be called on confirm'); }; | ||
export default class ConfirmStatusChange extends React.Component<Props, State> { | ||
|
||
public state = { |
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 and other methods that are only used in this class should probably not be marked public
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.
good point. It was tslint --fix
that added them and i should have probably done a second pass.
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.
looks like typescript is complaining about that. React.PureComponet and React.Component both expose state as a public property.
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.
oh react
open: false, | ||
}; | ||
|
||
public show: ShowCallback = (callback) => (event) => { |
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 think it would make more sense to pass the callback as a prop to the <ConfirmStatusChange>
component. The callback isn't really "state" as far as I can tell, and it could be misleading to think that you can call show
multiple times, when that will actually break things.
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.
The callback is held in the closure state until the event triggers at which point it's put into the components state and the modal pops up. Holding the callback in state only happens when the modal is open and we're just waiting for user confirmation before executing the original function. Show can be called multiple times with different callbacks and each callback won't be placed into the state until right before they're about to be executed/cancel (via the modal). Since only one modal can be open at a time I think this makes sense.
What isn't configurable is the message displayed to the user, so that does limit the number of confirmation prompts this can be used with. Passing the title and description to show
may make sense to enable multiple confirmation messages.
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.
Oh, I see. I stand corrected. I'm still a little confused by this though. What's the advantage of a stateful setup like this instead of using a separate ConfirmStatusChange
for each action that needs confirmation, and passing the callback in as a prop?
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.
Good point. I had modal reuse in mind when I was writing it, but that kind of fell apart once I had to use it in the actions cell renderer. I'll change it, you're right, it's less confusing to pass stuff as props.
} | ||
|
||
return ( | ||
<ConfirmStatusChange title={t('Please Confirm')} description={`${t('Are you sure you want to delete')} ${original.dashboard_title}?`}> |
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 <ConfirmStatusChange>
could wrap just the delete button instead of the entire collection of buttons.
this.setState({ | ||
callback: () => callback(event), | ||
callbackArgs, |
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.
If the callback args is supposed to be the DOM event, this will not work as intended. Events can't be referenced asynchronously. https://reactjs.org/docs/events.html#event-pooling
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.
You'd need to check if the arg is an Event, and call .persist()
on 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.
Good catch, I was cloning it before. None of my current use cases rely on the dom event, but probably best to handle that to avoid future weirdness.
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 like the hooks and good job DRYing things up!
CATEGORY
Choose one
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS