Adding Page component and related files #216
Conversation
tbiethman
requested review from
dkasper-was-taken,
emilyrohrbough,
mjhenkes and
ryanthemanuel
as code owners
| Page.propTypes = propTypes; | ||
| Page.Actions = PageActions; | ||
| Page.Action = PageAction; | ||
| Page.Toolbar = PageToolbar; |
There was a problem hiding this comment.
These might be a little controversial. As we have moved away from needing HOCs all over the place, the usage of static exports from some of our "umbrella" components becomes a little more straightforward and less error prone. The components themselves are documented separately so we can document prop tables and all that jazz.
I feel like this simplifies the consumer's implementation code and keeps these components (that are required for usage with the Page) bundled together nicely. Take a look at the example implementations and let me know your thoughts.
There was a problem hiding this comment.
I like the explicit bundling of the page and it's sub components. My only concern is introducing inconsistency regarding how we present other API's in the package, such as workspace. It may be worth our time to go back and implement this style of api for those other areas at least in the terra-application package.
There was a problem hiding this comment.
Agreed, I can log an investigative story for that as part of our v2 release. We could remain passive by keeping any existing explicit exports in place.
There was a problem hiding this comment.
Logged issue: #219
| * | ||
| * The PageActions component should not be rendered directly. | ||
| */ | ||
| const PageActions = () => <div />; |
There was a problem hiding this comment.
it doesn't matter, but could this be an empty fragment?
There was a problem hiding this comment.
Good question. It needs to be non-null so that the props table parsing will recognize it as a component, but I imagine a fragment would work too? I'll try it out.
| /** | ||
| * Identifier for use during tests | ||
| */ | ||
| testId: PropTypes.string, |
There was a problem hiding this comment.
testId? Should we make this private or just be a regular id?
There was a problem hiding this comment.
I don't think this is being used at the moment, so I'll remove. Should be able to query in tests based on role/display just fine.
| <PageHeader | ||
| id={pageId} | ||
| onSelectBack={onRequestClose ? () => { | ||
| if (dangerouslyDisableUnsavedChangesPromptHandling) { |
There was a problem hiding this comment.
(small and not a big deal) - we could convert this into a handleOnSelectBack function and pass it here to make the ternary slightly more readable
| )); | ||
| } | ||
|
|
||
| function renderMenuButton() { |
There was a problem hiding this comment.
what are your thoughts on further abstracting these render functions into their own components? Looks like we have ActionButtons, MenuButton, Menu, Toolbar, StartActions, EndActions. Extracting these out will remove nested rendering and make this component easier to follow
There was a problem hiding this comment.
Separate components would result in a lot of props being passed around and seems like overkill (for what would amount to a single-use component). But I might be able to make this more straightforward, I'll play with it a bit.
| <Popup | ||
| isOpen={showMenu} | ||
| targetRef={() => moreActionsButtonRef.current} | ||
| onRequestClose={() => { setShowMenu(false); }} |
There was a problem hiding this comment.
nitpick haha
| onRequestClose={() => { setShowMenu(false); }} | |
| onRequestClose={() => setShowMenu(false)} |
| label={intl.formatMessage({ | ||
| id: 'terraApplication.pageHeader.actionsMenu', | ||
| }, { label })} | ||
| onRequestClose={() => { setShowMenu(false); }} |
There was a problem hiding this comment.
if you update above, let's do the same here for consistency
| onRequestClose={() => { setShowMenu(false); }} | |
| onRequestClose={() => setShowMenu(false)} |
| * Use this prop to customize UnsavedChangesPrompt handling prior to Page | ||
| * closure. | ||
| */ | ||
| dangerouslyDisableUnsavedChangesPromptHandling: PropTypes.bool, |
There was a problem hiding this comment.
remind me, in what situations does this get used?
There was a problem hiding this comment.
This is the "I'm handling my Page's unsaved changes differently and don't want two prompts to pop up" prop. I'd anticipate something similar on the new Modal.
Summary
Adding the Page component, the main UI component intended for usage in the provided PageContainers.
Closes #193
Deployment Link
https://terra-applic-.herokuapp.com/
Testing
Additional Details
Theme values are not yet defined in total. A separate issue has been logged to get those implemented when the values are known: #218
Thank you for contributing to Terra.
@cerner/terra