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

Grafana UI: Add code variant to Text component #82318

Merged
merged 3 commits into from
Feb 19, 2024
Merged

Grafana UI: Add code variant to Text component #82318

merged 3 commits into from
Feb 19, 2024

Conversation

tskarhed
Copy link
Contributor

@tskarhed tskarhed commented Feb 12, 2024

What is this feature?

Adds a monospaced code variant to make it available in the Text component. code is used as it aligns somewhat with the design tokens.

image

Why do we need this feature?

Sometimes we need to use a monospaced font.

Who is this feature for?

Developers

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@tskarhed tskarhed requested review from a team as code owners February 12, 2024 15:02
@tskarhed tskarhed requested review from joshhunt, ashharrison90 and Ukochka and removed request for a team February 12, 2024 15:02
Comment on lines 22 to 23
/** The default variant is 'body'. To fit another styles set the correspondent variant as it is necessary also to adjust the icon size. `code` is excluded, as it is not fit for links. */
variant?: keyof Omit<ThemeTypographyVariantTypes, 'code'>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just my initial thoughts, I can be convinced otherwise

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it makes sense, but just double checking with @staton-hyse11 what his opinion is. We can also remove this Omit later if we find a use case for it and it won't be a breaking change

Copy link
Contributor

@JoaoSilvaGrafana JoaoSilvaGrafana left a comment

Choose a reason for hiding this comment

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

Lgtm! Do we want to add something to the documentation both in storybook and here about how if developers want to display code they should use this variant?.

Comment on lines 22 to 23
/** The default variant is 'body'. To fit another styles set the correspondent variant as it is necessary also to adjust the icon size. `code` is excluded, as it is not fit for links. */
variant?: keyof Omit<ThemeTypographyVariantTypes, 'code'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it makes sense, but just double checking with @staton-hyse11 what his opinion is. We can also remove this Omit later if we find a use case for it and it won't be a breaking change

@@ -29,7 +29,7 @@ interface TextLinkProps extends Omit<AnchorHTMLAttributes<HTMLAnchorElement>, 't
}

const svgSizes: {
[key in keyof ThemeTypographyVariantTypes]: IconSize;
[key in keyof Omit<ThemeTypographyVariantTypes, 'code'>]: IconSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could create a type at the top for this. Like type TextLinkVariants = keyof Omit<ThemeTypographyVariantTypes, 'code'>; and then we could use it both here and in line 23?

@tskarhed
Copy link
Contributor Author

tskarhed commented Feb 13, 2024

Lgtm! Do we want to add something to the documentation both in storybook and here about how if developers want to display code they should use this variant?.

I see you point, but I do think we should make a code component (if we don't already have one?) for writing inline code that has some border and highlighting. So in general the code variant shouldn't be used, unless there are some rare cases? 🤔

So maybe adding the code variant to Don't use unless you are really sure that you should?

@tskarhed tskarhed added the area/grafana/ui Issues that belong to components in the @grafana/ui library label Feb 14, 2024
@tskarhed tskarhed changed the title Text: Add code variant Grafana UI: Add code variant to Text component Feb 14, 2024
@JoaoSilvaGrafana
Copy link
Contributor

JoaoSilvaGrafana commented Feb 14, 2024

Lgtm! Do we want to add something to the documentation both in storybook and here about how if developers want to display code they should use this variant?.

I see you point, but I do think we should make a code component (if we don't already have one?) for writing inline code that has some border and highlighting. So in general the code variant shouldn't be used, unless there are some rare cases? 🤔

So maybe adding the code variant to Don't use unless you are really sure that you should?

Ah so wait do we want it to be a code variant or a more generic monospace variant? What's some examples where we would use this variant?

Ignore me, code seems more logical, but yeah maybe add a line like that saying it is discouraged unless there is a specific reasoning behind it.

Copy link
Collaborator

@codeincarnate codeincarnate left a comment

Choose a reason for hiding this comment

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

Saw this and Dataviz could potentially use this so decided to check it out. Seems to work well!

@eledobleefe
Copy link
Contributor

Lgtm! Do we want to add something to the documentation both in storybook and here about how if developers want to display code they should use this variant?.

I see you point, but I do think we should make a code component (if we don't already have one?) for writing inline code that has some border and highlighting. So in general the code variant shouldn't be used, unless there are some rare cases? 🤔

So maybe adding the code variant to Don't use unless you are really sure that you should?

Hi @tskarhed!
First of all, great job on this! 🙌
My 2 cents here... I would also add some notes to the documentation. Devs will see that option and, unless we guide them, they can end up using this as we do not want. Maybe just add kind of what you said: "There is a 'code' variant that will show a monospace typography, but it should be used only when there are x and x requirements", for example 🤷
Otherwise, LGTM 👌

@tskarhed tskarhed enabled auto-merge (squash) February 19, 2024 12:36
@tskarhed tskarhed merged commit 6ca26dd into main Feb 19, 2024
17 of 18 checks passed
@tskarhed tskarhed deleted the text-monospace branch February 19, 2024 12:49
@aangelisc aangelisc modified the milestones: 10.4.x, 11.0.x Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/grafana/ui Issues that belong to components in the @grafana/ui library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants