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

feat(css): add CSS display utilities #17359

Merged
merged 14 commits into from Feb 22, 2019
Merged

feat(css): add CSS display utilities #17359

merged 14 commits into from Feb 22, 2019

Conversation

seiyria
Copy link
Contributor

@seiyria seiyria commented Feb 1, 2019

Short description of what this resolves:

It adds features described in #10904 to have responsive visibility.

Changes proposed in this pull request:

  • add .ion-hide selector to hide content
  • add .ion-hide-{breakpoint}-{up|down}] to selectively hide content

This could probably be further enhanced with .ion-hide-{breakpoint}-only and .ion-show-{breakpoint}-only. Not sure how far to take this particular PR.

Docs PR

ionic-team/ionic-docs#403

Ionic Version:
4.0.0

Fixes: #10904

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Feb 1, 2019
@brandyscarney
Copy link
Member

Thanks for the PR!

@manucorporat Are you good with the naming of this file? It will be an optional file to include for display properties, like the float ones: https://github.com/ionic-team/ionic/blob/master/core/src/css/float-elements.scss

@brandyscarney brandyscarney changed the title Add CSS visibility utilities feat(css): add CSS visibility utilities Feb 12, 2019
@brandyscarney
Copy link
Member

This looks great by the way! I don't think we should add the -only right now only because the others (text-transformation, text-alignment) don't have it.

My only concern is if we need to add classes like the others do:

.ion-text#{$infix}-uppercase,
[text#{$infix}-uppercase] {
  /* stylelint-disable-next-line declaration-no-important */
  text-transform: uppercase !important;
}

so it would be like:

[hidden#{$infix}-up],
.ion-hidden#{$infix}-up {
  display: none !important;
}

I know this was added in other places because of react and type errors with JSX. cc @manucorporat @jthoms1

@seiyria
Copy link
Contributor Author

seiyria commented Feb 12, 2019

That's fair! I figured I would mention it and leave the choice of inclusion up to the team. Let me know of any changes that need to be made!

@brandyscarney
Copy link
Member

@seiyria Will do! Thanks again. 🙂

@brandyscarney
Copy link
Member

brandyscarney commented Feb 21, 2019

Okay I finally got some answers for you!

So we've decided that we're going to change the following few things:

  1. Change it from hidden to hide because of the native global attribute (https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden). The reason being is that you can have the following:

    <p hidden>
      I am hidden
    </p>
    p {
      display: block;
    }

    The paragraph would actually show because the display should take precedence, but our CSS rule will cause conflicts with that.

  2. Remove the attributes entirely and only use CSS classes. This is because the HTML attributes cause conflicts with typing in JSX and React because it assumes they should be defined properties on the element. So moving forward we would like to only use CSS classes for styling. The other ones will keep their attributes but we will be deprecating them (to eventually remove) and start to only document use of the CSS classes.

    .ion-hide {
      display: none !important;
    }
    
    // Adds display attributes
    @each $breakpoint in map-keys($screen-breakpoints) {
      $infix: breakpoint-infix($breakpoint, $screen-breakpoints);
    
      @include media-breakpoint-up($breakpoint, $screen-breakpoints) {
        // Provide `.ion-hide-{bp}-up` classes for hiding the element based
        // on the breakpoint
        .ion-hide-#{$infix}-up {
          display: none !important;
        }
      }
    
      @include media-breakpoint-down($breakpoint, $screen-breakpoints) {
        // Provide `.ion-hide-{bp}-down` classes for hiding the element based
        // on the breakpoint
        .ion-hide-#{$infix}-down {
          display: none !important;
        }
      }
    }

    Note: I know this voids the first point technically but on the off chance we ever do need attributes I'd rather it be the same.

  3. Rename the file and contents from visibility to display. I think this makes it more clear what they are used for since the visibility CSS property exists and that's not what we're styling: https://developer.mozilla.org/en-US/docs/Web/CSS/visibility

Let me know what you think!

@seiyria
Copy link
Contributor Author

seiyria commented Feb 21, 2019

This seems reasonable! I'll see if I can get some adjustments made today.

@brandyscarney
Copy link
Member

Very nice, thank you! So I plan on getting this in to the 4.1.0 release. We haven't merged features yet as we're working out the process behind it (which branch will hold features). Once that happens though I will get this in!

@seiyria
Copy link
Contributor Author

seiyria commented Feb 21, 2019

Fantastic! Thank you! I've also updated the docs PR to reflect these changes.

@brandyscarney
Copy link
Member

@liamdebeasi Could you add your review here?

@brandyscarney brandyscarney changed the title feat(css): add CSS visibility utilities feat(css): add CSS display utilities Feb 22, 2019
Copy link
Member

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for submitting 🎉

@brandyscarney brandyscarney merged commit 6bea9d3 into ionic-team:master Feb 22, 2019
@brandyscarney
Copy link
Member

Yay, merged! Thanks!

@seiyria
Copy link
Contributor Author

seiyria commented Feb 22, 2019

Thank you! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants