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

[FEATURE] Improved Visibility styles for responsive designs #427

Closed
3den opened this issue Mar 16, 2017 · 6 comments
Closed

[FEATURE] Improved Visibility styles for responsive designs #427

3den opened this issue Mar 16, 2017 · 6 comments

Comments

@3den
Copy link

3den commented Mar 16, 2017

Currently the visibility classes all add display: block, which breaks the grid, it is documented that we should create modifiers like .slds-medium-show--inline-block which is a hack to please the incorrect implementation. Also based on BEM the modifier should be the size, like we did for containers, padding, margins...

.slds-hide { display: none; }
.slds-show { /* nothing needed, will use the whatever is there */ }


@media(max-width: $mq-small - 1){
  // show: hides on sizes smaller than breakpoint
  .slds-show--small { display: none; }   
}

@media(min-width: $mq-small){
  // hide: hides on sizes larger than breakpoint
  .slds-hide--small { display: none; }   
}

We can use a loop to generate this classes so we don't need to duplicate code.

PS: I am willing to send a PR for this issue.

@shyamster
Copy link

+1

@kaelig
Copy link
Contributor

kaelig commented Mar 16, 2017

Related to: #263

@3den
Copy link
Author

3den commented Mar 17, 2017

@kaelig an other approach is so you can keep the current code working as it is is to just do responsive hides e.g. hide—min-small, hide—max-small, hide—max-large, hide—min-large

@3den
Copy link
Author

3den commented Mar 17, 2017

The current implementation cannot be used for responsive designs because it only hides what is smaller than the break point. For instance the code below wont work as expected:

<header>
  <nav class="slds-medium-hide">navbar for mobile</nav>
  <nav class="slds-medium-show">navbar for desktop</nav>
</header>

The code above will load both navbars on desktop, ands seems like there is no way to do this correctly on SLDS.

@3den
Copy link
Author

3den commented Mar 17, 2017

@kaelig also .slds-max-XXX-hide is confusing, and seems to be redundant, for instance if the documentation is correct .slds-max-small-hide does pretty much the same as .slds-small-show.

@ishakasliwal
Copy link
Contributor

Thank you for the PR and contribution @3den ! This work has been updated on our internal repo and will be made publicly available upon our next release.

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

No branches or pull requests

4 participants