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.

Sign up for GitHub

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

Jump to bottom

[Badge] Give Badge dynamic width and other improvements #14121

Merged
merged 5 commits into from Jan 14, 2019
Merged

[Badge] Give Badge dynamic width and other improvements #14121

merged 5 commits into from Jan 14, 2019

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Jan 8, 2019 •
edited

Closes #14105

Demo: https://deploy-preview-14121--material-ui.netlify.com/demos/badges/

Currently, it's a breaking change due to showZero but we can always invert this and call it hideZero and potentially revert back after v4

Added properties:

  • showZero
  • max
  • variant (dot and default)

To Do

  • Add tests

@joshwooding joshwooding force-pushed the badge-component branch 2 times, most recently from 1753645 to ed6f737 Compare January 8, 2019 23:00
@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 Jan 8, 2019
oliviertassinari
oliviertassinari reviewed Jan 8, 2019
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The animation of the Show Badge label in the documentation doesn't longer work.
There is some small breaking change here, depends on how we see it. I would say we should release it in a minor rather than a patch.

packages/material-ui/src/Badge/Badge.js Outdated Show resolved Hide resolved
packages/material-ui/src/Badge/Badge.js Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

one possible fix for the show/hide animation:

    transform: scale(0) translate(50%, -50%);
    transform-origin: 100% 0%;

@joshwooding joshwooding force-pushed the badge-component branch 2 times, most recently from 23698bd to e8a1d87 Compare January 9, 2019 00:07
@joshwooding joshwooding changed the title [Badge] Give Badge dynamic width and add showZero and max properties [Badge] Give Badge dynamic width and other improvements Jan 9, 2019
@oliviertassinari oliviertassinari self-assigned this Jan 13, 2019
@oliviertassinari oliviertassinari force-pushed the badge-component branch 2 times, most recently from 6f273c6 to ed19cf5 Compare January 13, 2019 12:11
@oliviertassinari oliviertassinari removed their assignment Jan 13, 2019
eps1lon
eps1lon requested changes Jan 13, 2019
View reviewed changes
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Currently, it's a breaking change due to showZero but we can always invert this and call it hideZero and potentially revert back after v4

Shouldn't this be targeted at next then?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 13, 2019 •
edited

I would call it a bug fix. What's the use case for showing a badge when it's zero?

@oliviertassinari oliviertassinari merged commit cfe4f60 into mui:master Jan 14, 2019
@mbrookes
Copy link
Member

What's the use case for showing a badge when it's zero?

If there isn't one, do we really need the showZero prop?

kLabz
kLabz reviewed Jan 15, 2019
View reviewed changes
packages/material-ui/src/Badge/Badge.js
let displayValue = '';

if (variant !== 'dot') {
displayValue = badgeContent > max ? `${max}+` : badgeContent;
Copy link
Contributor

Choose a reason for hiding this comment

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

So now badgeContent is not a node as specified by the TypeScript/PropTypes definitions, but must be a number?

Copy link
Member

Choose a reason for hiding this comment

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

@kLabz Do you have a specific issue using a node? 'hello' > 99 = false, ({}) > 99 = false

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really, but this feels really weird. If other node types are still considered supported, it's fine for me 👍

(Context: I am writing type definitions for a statically typed language so if the type changed I would have updated to the equivalent of number)

Copy link
Member

Choose a reason for hiding this comment

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

It's true that we don't have any test with a react node. I think that we should add one.

kLabz
kLabz reviewed Jan 15, 2019
View reviewed changes
packages/material-ui/src/Badge/Badge.js
@@ -95,7 +120,7 @@ Badge.propTypes = {
/**
* The content rendered within the badge.
*/
badgeContent: PropTypes.node.isRequired,
badgeContent: PropTypes.node,
Copy link
Contributor

Choose a reason for hiding this comment

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

TS should be updated too if this prop is not mandatory any more.

Copy link
Member

Choose a reason for hiding this comment

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

@kLabz Yes, it should. Do you want to take care of it? Well spotted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done (#14186, I hope I was supposed to target next)

@joshwooding joshwooding deleted the badge-component branch January 18, 2019 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrookes mbrookes mbrookes left review comments

@oliviertassinari oliviertassinari oliviertassinari approved these changes

@kLabz kLabz kLabz left review comments

@eps1lon eps1lon eps1lon requested changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@joshwooding @oliviertassinari @mbrookes @kLabz @eps1lon