-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor common dialog/drawer code into a base class #1801
Conversation
@atmgrifter00 would you mind buddying this? |
change/@ni-nimble-components-dc071140-b97c-473f-b929-1268a3aa3a40.json
Outdated
Show resolved
Hide resolved
Adding @mollykreis to take a look since they are familiar with original code and the issue being addressed |
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 didn't get a chance to review before going out of office. If other reviewers approve then you don't need to wait for me.
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.
@m-akinc FYI there are some lint errors but otherwise looks good!
this.startClosing(reason); | ||
} | ||
|
||
/** |
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 responsibilities of cancel and close handlers don't seem quite right. I think similar to some previous commits they seem over coupled to specific workflows. Modal instances like drawer calling "super.cancelHandler" to start the transition to Closing and check the preventDismiss state seems like unclear responsibilities for a method called "cancelHandler".
Looking at it again I also have a more general question of why are we not designing animations into the Modal base class. I'm actually wondering now if it is really intentional that the dialog has no animations. Seems like even short, fast animations would be useful for all modals.
Some directions:
- We can continue to discuss and iterate on the responsibilities of the base class, including seeing if animation responsibilities make more sense there, it probably makes sense to discuss it first to prevent review cycles or continue the approach of aync comment and feedback if you want to keep going that way
- We pause the refactor for now since we don't have a clear need for shared Modal base classes at the moment (I'm not aware of new modals we need to implement or if we want to share more behavior like animations across all the modals)
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'll de-prioritize and move to draft until I have new changes to review.
Closing the draft as unprioritized right now. Think it would be useful to reconsider if adding additional modal types / decide to share more functionality (i.e. animations for dialogs and drawers) |
Pull Request
π€¨ Rationale
Refactor to avoid duplication between dialog and drawer.
π©βπ» Implementation
Created common base class
DialogBase
.π§ͺ Testing
Existing tests pass.
β Checklist