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

Adding MainContainer, ActiveMainContext, and related files #195

Merged
merged 12 commits into from Apr 26, 2021

Conversation

tbiethman
Copy link
Contributor

Summary

Adding the MainContainer here, which provides an easy way for consumers to render accessible main content. Also added is infrastructure to communicate the applications current active main element, with integration into our navigation architecture. This is primarily used by Workspace but could have other applications in the future.

Closes #156.

Deployment Link

https://terra-applic-.herokuapp.com/

Testing

Unit and functional tests added for new functionality.

Additional Details

Thank you for contributing to Terra.
@cerner/terra

…ation into story-156-active-main-page

# Conflicts:
#	CHANGELOG.md
#	src/application-container/ApplicationContainer.jsx
#	src/application-container/private/skip-to-links/SkipToLink.jsx
#	tests/wdio/test-harness/index.jsx
#	translations/en-US.json
#	translations/en.json
@tbiethman tbiethman self-assigned this Apr 23, 2021
@mjhenkes mjhenkes temporarily deployed to terra-applic-story-156--z8dzvb April 23, 2021 16:38 Inactive
@mjhenkes mjhenkes temporarily deployed to terra-applic-story-156--yc2guj April 23, 2021 18:03 Inactive
@mjhenkes mjhenkes temporarily deployed to terra-applic-story-156--yc2guj April 23, 2021 18:38 Inactive
@mjhenkes mjhenkes temporarily deployed to terra-applic-story-156--yc2guj April 26, 2021 14:12 Inactive
});

return () => {
dispatch({ type: 'unregister', registrationId });
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick for readability/consistency

Suggested change
dispatch({ type: 'unregister', registrationId });
dispatch({
type: 'unregister',
registrationId,
});

@@ -0,0 +1,5 @@
:local {
.main-container {
outline: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

should main have 100% height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this but erred on the side of caution. If we add a height, I feel like we'd need to make the same call for overflow handling. But that is not always desired/required (for the Page use case, or example), which opens the door to all sorts of props to disable/enable things...

Long story long I'd like to avoid adding more styles here, for the time being at least. Makes me wonder if we should add a MainContainer to the eventual LegacyLayout and provide the default styling we used to provide there while keeping this component more generic.

Comment on lines +47 to +50
// If an ancestor provider exists, we need to forward the active main info
// if the provider exists in the active navigation branch. It is
// otherwise unregistered, if necessary, as we do not want potentially stale
// information living above this provider level.
Copy link
Contributor

Choose a reason for hiding this comment

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

Having nested active main providers should never be a thing right? If that happens maybe we should just blow up?

Copy link
Contributor Author

@tbiethman tbiethman Apr 26, 2021

Choose a reason for hiding this comment

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

It is a thing, as the ActiveMainProviders are rendered both by the ApplicationContainer as well as by each NavigationItem (which will be inside the ApplicationContainer, and potentially nested even further if we have secondary/tertiary navigation concepts).

The nested providers are what allow us to limit cross-talk between different navigation branches of the application.

@mjhenkes mjhenkes temporarily deployed to terra-applic-story-156--yc2guj April 26, 2021 18:25 Inactive
@tbiethman tbiethman merged commit 52332e9 into terra-application-v2 Apr 26, 2021
@tbiethman tbiethman deleted the story-156-active-main-page branch April 26, 2021 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants