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

pkg/ui: Don't force tracez tags to uppercase. #83913

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

benbardin
Copy link
Collaborator

Also, deprecate uses of db-console/.../Badge in favor of the identical version
in cluster-ui.

Screen Shot 2022-07-06 at 1 21 50 PM

Release note: None

@benbardin benbardin requested review from andreimatei, knz, adityamaru and a team July 6, 2022 17:23
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

thanks

Copy link
Collaborator Author

@benbardin benbardin left a comment

Choose a reason for hiding this comment

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

You're welcome @knz ! CI is passing now, can I get an LGTM? Thank you!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @andreimatei)

@knz
Copy link
Contributor

knz commented Jul 8, 2022

I'm OK with the change, but I'm not technology expert on the front-end.
Do you want to get your code review from someone with front-end expertise? For example @nathanstilwell , @dhartunian or @koorosh

Copy link
Collaborator Author

@benbardin benbardin left a comment

Choose a reason for hiding this comment

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

Can do!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @andreimatei)

@benbardin benbardin requested review from dhartunian and koorosh July 8, 2022 14:48
@nathanstilwell nathanstilwell self-requested a review July 8, 2022 15:05
@nathanstilwell
Copy link
Contributor

pkg/ui/workspaces/cluster-ui/src/badge/badge.tsx line 35 at r1 (raw file):

    styles.push("badge--uppercase");
  }
  const classes = cx(...styles);

So classnames already has the functionality you are creating here (the ability to add a className based on a boolean variable). So I think what you want is something like,

const classes = cx("badge", `badge--size-${size}`, `badge--status-${status}`{
    "badge--uppercase": !allowLowerCase,
  });

This actually makes this kind of confusing for me to read since we are applying the badge--uppercase class after negating a variable. It seems like the default for badges should be that they are uppercase and we want the ability to override that. So what if we had a prop called something like uppercase and defaulted it to false? Then the cx call would look like,

const classes = cx("badge", `badge--size-${size}`, `badge--status-${status}`{
    "badge--uppercase": uppercase,
  });

giving us something like,

export function Badge(props: BadgeProps) {
  const { size, status, icon, iconPosition, text, uppercase = true } = props;
  const classes = cx("badge", `badge--size-${size}`, `badge--status-${status}`{
    "badge--uppercase": uppercase,
  });

Anyway, just a thought.

Code quote:

  const { size, status, icon, iconPosition, text, allowLowerCase } = props;
  const styles = ["badge", `badge--size-${size}`, `badge--status-${status}`];
  if (!allowLowerCase) {
    styles.push("badge--uppercase");
  }
  const classes = cx(...styles);

@nathanstilwell
Copy link
Contributor

Thanks for cutting out a duplicate! I think I would prefer that we use Badge from @cockroachlabs/ui-components everywhere, but looking at the implementation it differs slightly from what's in this repo and doesn't support all the features that have been added in Figma for badges (which makes me suspect that this implementation may differ from what's in Managed Service). All that to say, we can worry about migrating to the UI repo another time.

Thank you.

Copy link
Collaborator Author

@benbardin benbardin left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, @dhartunian, @koorosh, and @nathanstilwell)


pkg/ui/workspaces/cluster-ui/src/badge/badge.tsx line 35 at r1 (raw file):

Previously, nathanstilwell (Nathan Stilwell) wrote…

So classnames already has the functionality you are creating here (the ability to add a className based on a boolean variable). So I think what you want is something like,

const classes = cx("badge", `badge--size-${size}`, `badge--status-${status}`{
    "badge--uppercase": !allowLowerCase,
  });

This actually makes this kind of confusing for me to read since we are applying the badge--uppercase class after negating a variable. It seems like the default for badges should be that they are uppercase and we want the ability to override that. So what if we had a prop called something like uppercase and defaulted it to false? Then the cx call would look like,

const classes = cx("badge", `badge--size-${size}`, `badge--status-${status}`{
    "badge--uppercase": uppercase,
  });

giving us something like,

export function Badge(props: BadgeProps) {
  const { size, status, icon, iconPosition, text, uppercase = true } = props;
  const classes = cx("badge", `badge--size-${size}`, `badge--status-${status}`{
    "badge--uppercase": uppercase,
  });

Anyway, just a thought.

Oh, neat trick! Thanks! How's this look?

@nathanstilwell
Copy link
Contributor

pkg/ui/workspaces/cluster-ui/src/badge/badge.tsx line 35 at r1 (raw file):

Previously, benbardin (Ben Bardin) wrote…

Oh, neat trick! Thanks! How's this look?

I still find reviewable very confusing, but I'm not seeing a change yet. Am I missing something?

Copy link
Collaborator Author

@benbardin benbardin left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, @dhartunian, @koorosh, and @nathanstilwell)


pkg/ui/workspaces/cluster-ui/src/badge/badge.tsx line 35 at r1 (raw file):

Previously, nathanstilwell (Nathan Stilwell) wrote…

I still find reviewable very confusing, but I'm not seeing a change yet. Am I missing something?

No, I just forgot to commit locally before pushing 🤦 How about now?

@nathanstilwell
Copy link
Contributor

pkg/ui/workspaces/cluster-ui/src/badge/badge.tsx line 35 at r1 (raw file):

Previously, benbardin (Ben Bardin) wrote…

No, I just forgot to commit locally before pushing 🤦 How about now?

Nice.

Copy link
Contributor

@nathanstilwell nathanstilwell left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, @dhartunian, and @koorosh)

Also, deprecate uses of db-console/.../Badge in favor of the identical version
in cluster-ui.

Release note: None
@benbardin
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 11, 2022

Build succeeded:

@craig craig bot merged commit bef1101 into cockroachdb:master Jul 11, 2022
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.

4 participants