carbon-design-system / carbon

A design system built by IBM
https://www.carbondesignsystem.com
Apache License 2.0
7.84k stars 1.81k forks source link

Shell and Content #5589

Closed ghost closed 4 years ago

ghost commented 4 years ago

Following the Shell setup from the tutorial, I have come to understand the the sidenav fixes itself at a certain view width. However, the Content component still sits underneath. Is there anyway to move the content such that the fixed sidenav doesn't cover it?

asudoh commented 4 years ago

Hi 👋 if you inspect the style of <div class="bx--col-lg-13 bx--offset-lg-3"> in https://carbon-components-react.netlify.com/iframe.html?id=ui-shell--header-base-w-navigation-actions-and-sidenav with Chrome DevTools, you'll see that margin-left changes at 66rem breakpoint. Do you want to do the same?

ghost commented 4 years ago

Well if that's the default behavior of the component, I don't think I have much of a choice.

My primary problem is that using a CSS grid class is a bad idea because it's based on a percentage of the view width, just like any other grid system, which would basically make the content start (i.e. it's left edge) at varying positions on different view width. That being said the Content component should adjust to accommodate the exact width of the sidenav. I'm not sure what good is a Content component that has no functionality. It's essentially just a div.

asudoh commented 4 years ago

@carbon-design-system/design Any thought on responsible behavior of the content next to side nav...? Thanks!

ghost commented 4 years ago

Like I said I think the Content component should have a purpose. Correct me if I'm wrong but it looks like it does absolutely nothing different than a div would do. I believe Content should respond to the same break points as sidenav does. If the screen is opened beyond the breakpoint, Content should set left: width-of-sidenav and width: calc(100% - width-of-sidenav) ... obviously I'm sure there's more to consider given different uses of the global app bar. But in any case, it makes no sense to make the header navigation disappear at the breakpoint and fix the sidenav without making the trifecta complete with the Content component, and make that adjust as well. Or at least give us the ability to customize the functionality via props.

chrisconnors-ibm commented 4 years ago

If you look at www.carbondesignsystem.com, the side nav only overlays content when it's a dismissable menu at the narrower breakpoint: overlay

At larger breakpoints when a permanent side nav is possible it influences the grid and moves content over. image

ghost commented 4 years ago

So what composition is used there? Take a look at the sidenav example in the docs and remove the case grid class and see what happens

ghost commented 4 years ago

Also in the shell in the docs you can click outside of the sidebar to close it whereas you can't in the example

ghost commented 4 years ago

https://codesandbox.io/s/header-with-sidenav-u2f53

ghost commented 4 years ago

@chrisconnors-ibm Any ideas?

chrisconnors-ibm commented 4 years ago

this is definitely wrong: image

Maybe @asudoh or another dev can take a look at your codesandbox and see if it's properly set up.

mjabbink commented 4 years ago

in Gatsby theme sites the left nav does not need to push content over as the nav has dedicated space i(n the larger breakpoints) allowing the main content to just simply live in the leftover columns.

asudoh commented 4 years ago

So, wrt the design discussion it seems that we are on the same page given what @chrisconnors-ibm and @mjabbink described matches https://carbon-components-react.netlify.com/iframe.html?id=ui-shell--header-base-w-navigation-actions-and-sidenav. @jibberishclient opened #5611 to ask for a component as an OOTB solution for changing margin upon side nav open/closed/rail states - So sounds that we can close this unless anybody disagrees.

ghost commented 4 years ago

@chrisconnors-ibm That's exactly the problem. According to the docs, the only thing pushing the content is a CSS grid class bx--col-lg-13 bx--offset-lg-3 which I've purposely omitted because it's A. an inconsistent hack job that causes different results on different view widths B. to address the complete lack of functionality of Content.

If both SideNav and HeaderNavigation respond to the breakpoint according to the design spec, then why doesn't the Content as well in conjunction with the others? It makes no sense to have a component with no functionality, as if a div wouldn't cut it. Nor does it make sense to rely on a percentage based, grid number to push content when the SideNav it has to accommodate is a fixed width, and thus so should the left margin.

ghost commented 4 years ago

The problem at large with carbon if I must be honest is the lack of consistency between comments here on Github, component docs, READMEs, and actual component usage/API in the latest version. Why does the docs website operate so much different from the components in the latest API version? Furthermore, it would be nice to have some kind of a slack for carbon where we can actually discuss this instead of sifting thru meaningless Github issues like Github is an HR department. It's much better to have discussions on these matters.

tw15egan commented 4 years ago

It looks like the styles that should be causing the content to shift are not being hit: https://github.com/carbon-design-system/carbon/blob/master/packages/components/src/components/ui-shell/_content.scss#L27-L33

Since SideNav is nested inside of Header, it is not adjacent to Content. Seems like the issue is resolved if SideNav is moved outside of Header, and adjacent to Content.

Content cannot simply respond via breakpoint, as it relies on adjusting whether or not the side nav is expanded or not, or if it is the Rail version of side nav where it will always have a thin gutter visible.

ghost commented 4 years ago

@tw15egan I copied and pasted from the docs aside from the CSS grid class bx--col-lg-13 bx--offset-lg-3 for the reasons described above. Is there any reason why the styles wouldn't apply?

tw15egan commented 4 years ago

Styles are not being applied because SideNav is nested inside Header. The styles are written in a way that suggests the SideNav should live adjacent to Content, not under Header

ghost commented 4 years ago

So are you suggesting the docs are outdated and the example is incompatible with new styles?

tw15egan commented 4 years ago

That's what it seems like. The way the styles are written, the only way Content will move over is if SideNav is adjacent to it, which is not how it is shown in any of the Header examples. Wondering if the examples just need to be updated to not have the SideNav nested. Like I said, your example correctly moves based on the SideNav if I simply move the SideNav next to Content

ghost commented 4 years ago

Could you throw up a quick fork of that? I'm not seeing it as of now.

tw15egan commented 4 years ago

Wrong example

ghost commented 4 years ago

Ya I mean I literally have 10 issues open for this singular problem because I was just kinda dismissed and told to refer to the docs, even though I've clearly reiterated many times that the docs are clearly VERY outdated. It really is such a same because carbon is arguably the best, most carefully designed component lib for React but the lack of proper documentation is such a turn off to be honest

ghost commented 4 years ago

That's trippyyyyyyyy. The shift only happens when I'm hovering over the SideNav. It also pushes the content on the smaller view, which it shouldn't. I don't think this is the right effect.

tw15egan commented 4 years ago

Ah right, on my laptop and was only checking smaller views. Okay, so then that isn't it.

I think it would be really helpful if we could consolidate all of these issues you've created into a checklist of actionable problems that you have been running in to. I'm having a tough time trying to figure out what exactly is differing from docs, and whether or not docs need to be updated or component code. needs to be fixed.

It would be best if you could follow the issue template and include links to CodeSandbox that illustrate the exact problems you're running into so we can help out better

ghost commented 4 years ago

To be honest, I think we should do this the other way around. I'll give you an "MVP" and challenge you to make a quick demo in CodeSandbox. It's probably just gonna be easier to "reverse-engineer" from the perspective of a carbon team member.

The goal is to create a sidebar which fixes to the side upon exceeding that breakpoint. However, the content should not be obstructed when this happens. DO NOT use any CSS grid class to move the content. On smaller screens below the breakpoint, the sidenav should toggle overlayed on top.

tw15egan commented 4 years ago

So, wrt the design discussion it seems that we are on the same page given what @chrisconnors-ibm and @mjabbink described matches https://carbon-components-react.netlify.com/iframe.html?id=ui-shell--header-base-w-navigation-actions-and-sidenav. @jibberishclient opened #5611 to ask for a component as an OOTB solution for changing margin upon side nav open/closed/rail states - So sounds that we can close this unless anybody disagrees.

Closing this in favor of #5611, which is a request for OOTB solution for handling margin on the Content component

victor-grajski commented 2 years ago

Please update the documentation on this! https://www.carbondesignsystem.com/guidelines/2x-grid/implementation#incorporating-the-shell