Skip to content
This repository was archived by the owner on May 24, 2024. It is now read-only.

Add ability to render layout at full page height #124

Closed
1 of 4 tasks
bjankord opened this issue Mar 22, 2018 · 8 comments
Closed
1 of 4 tasks

Add ability to render layout at full page height #124

bjankord opened this issue Mar 22, 2018 · 8 comments

Comments

@bjankord
Copy link
Contributor

bjankord commented Mar 22, 2018

Issue Description

For the terra-layout / terra-application-layout to render at full page height as expected, it's parent containers must be set to have height:100%; or its direct parent musth have height: 100vh;

However, I know there have been some concerns about setting those styles by default, as they conflict with the xfc / embedded-content-consumer styling.

Using the following config:

<BrowserRouter>
  <Base locale="en-US">
    <ApplicationLayout
      indexPath={indexPath}
      nameConfig={nameConfig}
      navigationItems={navigationItems}
      routingConfig={routingConfig}
    />
  </Base>
</BrowserRouter>

Results in a layout that is only as tall as the header.
menu

Looking at the generated markup, both the div#root and the direct child div rendered by the <Base /> component need height: 100% set to render at the full page height, or the modal manager needs its height changed from 100% to 100vh

screen shot 2018-03-22 at 12 05 32 pm

Issue Type

  • New Feature
  • Enhancement
  • Bug
  • Other

Expected Behavior

Ability to render layout at full page height without custom styles.

Current Behavior

Custom styles are required to render layout at full page height.

@bjankord
Copy link
Contributor Author

bjankord commented Mar 22, 2018

One possible option would be to add a fill attribute to the base component similar to the content container that when set, would style the div it renders as well as #root with height: 100%. This would be opt-in behavior as not to conflict with the xfc / embedded-content-consumer styling.

I'm also curious with that fill attribute on base, if we can just set 100vh on the div base renders and then not have to worry about trying to style the mounting node.

@dkasper-was-taken
Copy link
Contributor

100vh doesn't work across mobile devices, so the fill option would be better.

@bjankord bjankord added this to the Q2 2018 milestone Mar 22, 2018
@tbiethman
Copy link
Contributor

We're going to immediately run into the same issue with the ThemeProvider, which provides wraps content in yet another div that would need similar fill styles.

If we try and chase down all these wrappers and add fill props to them in order to get appropriate application container sizing, we're just changing one complexity for another.

Its seems like it would make sense to either include the ThemeProvider (and anything introduced in the future) with Base by default so that Base can style it. Or we could make another consolidation component that puts Base and the others together, but it seems like that's what Base is trying to do already.

We really need a one-stop shop if we want it to be easy to consume, and the one-stop shop should be the single component enforcing the fill styles.

@bjankord
Copy link
Contributor Author

Including the theme provider within base makes sense to me.

@bjankord
Copy link
Contributor Author

Talked at scrum, plan is to use terra-theme-provider in the new terra-application component and manage the theme concern at that level instead of terra-base.

In terra-base, we discussed using React.Fragmet instead of divs to get rid of the need for height: 100% to be set on the div rendered in terra-base.

@bjankord
Copy link
Contributor Author

I've hit a snag with replacing the div rendered via terra-i18n's <I18nProvider /> component with <React.Fragment>. We set a data attribute on it in terra-base. This data attribute is used to help solve accessibility issues with some components in terra-core and terra-framework.

The data-terra-base attribute is used to work around an issue on VoiceOver on iOS and focus traps. The focus trap we use, react-focus-trap, does not prevent users using VoiceOver on iOS from moving focus outside of the focus trap when the use the swipe gesture to navigate. This was an issue with modals, popups, and overlays where the user could move their focus outside of the opened component which is not ideal.

To mitigate this, what we've done is allow the user to provide a selector to be used with querySelector that will map to the "rootNode" via a prop. By default, we set this to [data-terra-base]. The idea is that when a modal or popup is opened, they are rendered in a portal. We use the rootNode to add an aria-hidden attribute to the "rootNode" which hides that part of the DOM, leaving the portalled DOM unhidden. This resolves the focus trap issue with VoiceOver on iOS.

Here is what the DOM looks like when the abstract modal is open.
Screen Shot 2019-03-20 at 11 06 52 AM

Given this, I still think it makes sense to remove the data-terra-base attribute and switch to using <React.Fragment />.

In the case of the rootNode, there was an assumption that we could default it to [data-terra-base]. I think the rootNode prop idea can still work, we'd just need to change it to what we default it to. I'd recommend we default the prop to #root. If teams need the rootNode prop we use to apply aria-hidden to be a different element, they can change the selector they pass to the rootNode.

@tbiethman
Copy link
Contributor

That sounds good to me. I think loosing that div in base and the associated styling concerns are worth the tradeoff. We'll just have to find a good place to document those selector expectations.

@bjankord bjankord added the Major Version Bump This issue requires a major version bump to the associated packages label Mar 21, 2019
@bjankord bjankord removed the Major Version Bump This issue requires a major version bump to the associated packages label Apr 16, 2019
@bjankord
Copy link
Contributor Author

This has been resolved with cerner/terra-core#2288

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants