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

fix(Hidden): Add Hidden component #103

Merged
merged 13 commits into from
Feb 25, 2019
Merged

fix(Hidden): Add Hidden component #103

merged 13 commits into from
Feb 25, 2019

Conversation

hscgavin
Copy link
Contributor

@hscgavin hscgavin commented Feb 22, 2019

Example usage

Hidden on mobile:

<Hidden mobile>
  <Text>Hidden on mobile</Text>
</Hidden>

Hidden on desktop:

<Hidden desktop>
  <Text>Hidden on desktop</Text>
</Hidden>

Hidden inline:

<Text>The end of this sentence is <Hidden inline mobile>hidden on mobile.</Hidden></Text>

Hidden on print:

<Hidden print>
  <Text>Hidden on print</Text>
</Hidden>

Hidden on screen:

<Hidden screen>
  <Text>Hidden on screen</Text>
</Hidden>

@michaeltaranto michaeltaranto changed the title feat(Hidden): Add Hidden component fix(Hidden): Add Hidden component Feb 22, 2019
label: 'Hidden on mobile',
render: () => (
<Hidden component="span" hideFor="mobile">
<Text baseline={false}>Hidden on mobile</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

The baseline={false} is not really required, it's best to try and keep these docs code blocks as lean as possible.

return (
<Box
display={[
hideFor === 'mobile' ? 'none' : 'inline',
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the reason for falling back to inline on mobile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so children is the one decide the display style display: xxx, if falling back to block, children couldn't override it? thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

We had to pass something there and inline is something. If the child is block it will still behave as expected.

Better ideas gratefully accepted 😅

@markdalgleish markdalgleish requested a review from a team as a code owner February 22, 2019 06:00
@markdalgleish
Copy link
Member

Just pushed an update that adds support for print/screen hiding, which necessitated a change in the API towards boolean props, since you can mix and match.

For example:

<Hidden desktop print>...</Hidden>

)}
/>
</button>
<Hidden print>
Copy link
Contributor

Choose a reason for hiding this comment

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

Dogfooding straight away 👍 🐶 🥘

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, trying to make this more of a habit.

Copy link
Contributor

@mengtzu mengtzu left a comment

Choose a reason for hiding this comment

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

Looks good. I was a bit surprised by the pivot back to boolean props but the mix and match use case seems persuasive.

screen?: boolean;
print?: boolean;
inline?: boolean;
component?: ReactType;
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed maybe remove the component option for now.

Copy link
Member

Choose a reason for hiding this comment

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

Done 👍

@markdalgleish markdalgleish merged commit 7879790 into master Feb 25, 2019
@markdalgleish markdalgleish deleted the feature/Hidden branch February 25, 2019 23:58
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

Successfully merging this pull request may close these issues.

5 participants