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

Conversation

3 participants
@seiyria
Copy link
Contributor

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

seiyria added some commits Feb 1, 2019

@ionitron-bot ionitron-bot bot added the package: core label Feb 1, 2019

@seiyria seiyria referenced this pull request Feb 1, 2019

Merged

add display to the docs #403

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

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 brandyscarney added this to In progress 🤺 in Ionic Core via automation Feb 12, 2019

@brandyscarney brandyscarney moved this from In progress 🤺 to Needs review 🤔 in Ionic Core Feb 12, 2019

brandyscarney added some commits Feb 12, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

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

@brandyscarney brandyscarney requested a review from manucorporat Feb 12, 2019

@seiyria

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@seiyria Will do! Thanks again. 🙂

@brandyscarney

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

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

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

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

Show resolved Hide resolved core/.stylelintrc.yml Outdated

seiyria and others added some commits Feb 22, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@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

@liamdebeasi
Copy link
Member

left a comment

Looks great! Thanks for submitting 🎉

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

1 check passed

build Workflow: build
Details

Ionic Core automation moved this from Needs review 🤔 to Done 🎉 Feb 22, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Yay, merged! Thanks!

@seiyria

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Thank you! 🎉

@seiyria seiyria deleted the seiyria:visibility-css branch Feb 22, 2019

KillerCodeMonkey added a commit to KillerCodeMonkey/ionic that referenced this pull request Mar 1, 2019

feat(css): add CSS display utilities (ionic-team#17359)
Adds classes to set display none on any element.

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

closes ionic-team#10904
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.