Skip to content
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

Consolidate *Dialog props and documentation; export Dialog #941

Merged
merged 6 commits into from
Apr 12, 2023

Conversation

lyzadanger
Copy link
Contributor

This PR consolidates props naming, organization and documentation for Dialog and ModalDialog, and adds Dialog to the package API1. It:

  • Adjusts the naming of a few props to better match conventions and make migration from Modal easier
  • Completes pattern-library documentation for both components and adds migration notes
  • Exports Dialog from the package (next) entrypoint (not ModalDialog, yet, because of some deficiencies in the useTabKeyNavigation hook when combined with embedded interactive widgets like DataTable and TabList.

Regarding the changing of a few prop names:

  • size -> (back to) width: I got a little zealous early on in the implementation of the new ModalDialog and wanted to start moving towards the standard theme-/customization-related prop names I presented to the team several weeks ago. However, it's distracting right now and will make migration from Modal more tedious. Change this prop name back to width so that it is unchanged from Modal.
  • The following boolean props were renamed on ModalDialog because their defaults differ from Dialog, and our conventions suggest that booleans should default false and be named for their effect when set (true):
    • closeOnEscape => disableCloseOnEscape
    • restoreFocus => disableRestoreFocus

Footnotes

  1. Holding for now on adding ModalDialog to API until some nuances with trapped-focus and embedded interactive widgets (e.g. DataTable, TabList) can be perfected.

Keep this prop name consistent with existing `Modal` component for
easier migration.
Our conventions suggest that boolean props should typically have a
default value of `false`, and be named for their behavior when `true`.

The props affected here have different defaults on `Dialog` than
`ModalDialog`, and are renamed accordingly.
@lyzadanger lyzadanger requested a review from acelaya March 29, 2023 16:49
@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #941 (7d2b2fa) into main (943eb1b) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #941   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           65        65           
  Lines          880       880           
  Branches       313       313           
=========================================
  Hits           880       880           
Impacted Files Coverage Δ
src/components/feedback/ModalDialog.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@acelaya
Copy link
Contributor

acelaya commented Mar 30, 2023

size -> (back to) width: I got a little zealous early on in the implementation of the new ModalDialog and wanted to start moving towards the standard theme-/customization-related prop names I presented to the team several weeks ago. However, it's distracting right now and will make migration from Modal more tedious. Change this prop name back to width so that it is unchanged from Modal.

Idea: Keep both width and size, marking width as deprecated. This will allow a more direct migration, but highlight the fact that this is not the path we want in the future.

Then inside the component we can do something in the lines of const size = props.size ?? props.width ?? <default_value_if_any>.

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@lyzadanger
Copy link
Contributor Author

size -> (back to) width: I got a little zealous early on in the implementation of the new ModalDialog and wanted to start moving towards the standard theme-/customization-related prop names I presented to the team several weeks ago. However, it's distracting right now and will make migration from Modal more tedious. Change this prop name back to width so that it is unchanged from Modal.

Idea: Keep both width and size, marking width as deprecated. This will allow a more direct migration, but highlight the fact that this is not the path we want in the future.

Then inside the component we can do something in the lines of const size = props.size ?? props.width ?? <default_value_if_any>.

This is a sensible idea, thank you. I've pushed another commit that does exactly this. Docs are updated also.

@lyzadanger lyzadanger requested a review from acelaya April 12, 2023 13:54
@acelaya
Copy link
Contributor

acelaya commented Apr 12, 2023

This is a sensible idea, thank you. I've pushed another commit that does exactly this. Docs are updated also.

Looks good :)

@lyzadanger lyzadanger merged commit 5297be5 into main Apr 12, 2023
@lyzadanger lyzadanger deleted the export-dialog branch April 12, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants