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

[Badge] Add invisible property #13534

Merged
merged 5 commits into from Nov 7, 2018

Conversation

joshwooding
Copy link
Member

Fixes #10466

  • [Badge] Added hide property
  • [Badge] Added hide property tests
  • [docs] Added hide property demo

[Badge] Added hide property tests
[docs] Added hide property demo
@@ -34,6 +34,7 @@ export const styles = theme => ({
backgroundColor: theme.palette.color,
color: theme.palette.textColor,
zIndex: 1, // Render the badge on top of potential ripples.
transition: '225ms cubic-bezier(0.4, 0, 0.2, 1)',
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the transition theme helper?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't even realise the theme has a transition helper. I'll give this a go 👍

@@ -50,6 +51,14 @@ export const styles = theme => ({
backgroundColor: theme.palette.error.main,
color: theme.palette.error.contrastText,
},
/* Styles applied to the badge `span` element if `hide={false}`. */
shown: {
Copy link
Member

Choose a reason for hiding this comment

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

Most of the time, we only provide a class rule for the non-default state. I think that we can merge it with root.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay :)

transform: 'scale(1)',
},
/* Styles applied to the badge `span` element if `hide={true}`. */
hidden: {
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to have the class rule match the property name. Let's find another wording that matches this requirement and that can be used as an adjective rather than only a verb.

Copy link
Member

Choose a reason for hiding this comment

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

cc @mbrookes I'm out of ideas.

Copy link
Member

@mbrookes mbrookes Nov 7, 2018

Choose a reason for hiding this comment

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

Tricky. inactive, closed, invisible?

Copy link
Member

Choose a reason for hiding this comment

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

invisible sounds good to me.

@oliviertassinari oliviertassinari added new feature New feature or request component: badge This is the name of the generic UI component, not the React module! labels Nov 6, 2018
[Badge] Merged shown class into root class
@joshwooding
Copy link
Member Author

joshwooding commented Nov 7, 2018

@oliviertassinari I know this isn’t a Badge specific issue but is there a way to stop the command yarn docs:api:core from changing the doc import statement’s path?

[docs] Updated Docs with Badge property change
@oliviertassinari
Copy link
Member

I know this isn’t a Badge specific issue but is there a way to stop the command yarn docs:api:core from changing the doc import statement’s path?

@joshwooding Are you on Windows? It can be a bug.

@joshwooding joshwooding changed the title [Badge] Add hide property [Badge] Add invisible property Nov 7, 2018
@joshwooding
Copy link
Member Author

I know this isn’t a Badge specific issue but is there a way to stop the command yarn docs:api:core from changing the doc import statement’s path?

@joshwooding Are you on Windows? It can be a bug.

Yeah, Looks like it I've submitted: #13541

@oliviertassinari oliviertassinari merged commit 48d6076 into mui:master Nov 7, 2018
@oliviertassinari
Copy link
Member

@joshwooding You did great here :)

@joshwooding joshwooding deleted the badge-hide-property branch November 7, 2018 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: badge This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants