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

[web] Allow providing ReactNodes as Table header text #48545

Closed
wants to merge 1 commit into from

Conversation

kiosion
Copy link
Contributor

@kiosion kiosion commented Nov 6, 2024

Related to #5075. Allow providing ReactNodes as Table header text, e.g. so icons can be used as header text.

@kiosion kiosion added the no-changelog Indicates that a PR does not require a changelog entry label Nov 6, 2024
@github-actions github-actions bot requested review from gzdunek and ryanclark November 6, 2024 20:36
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48545.d3pp5qlev8mo18.amplifyapp.com

- Allow providing `ReactNode`s as Table header text, e.g. so icons can be used
  as header text.
Copy link
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

I remember that some time ago I wanted to do something similar: allow string or ReactNode as notification content.

I found a comment from Rafał explaining why it was not the best idea:

I have to say I don't like this solution because it replicates the same problem that I mentioned when we were talking about using Markdown to insert the link: it shifts the responsibility of formatting the notification from the place which displays the notification to the place which wants the notification to be displayed.

If we add support for arbitrary HTML tags, if you want to answer the question of "How do all possible variants of notifications may look like, more or less?", you're no longer able to answer this question without grepping through all places in the codebase which add notifications because each one of them can do some custom stuff.

I think a better play here would be to add an optional link field to NotificationItemObjectContent. link could be an object with fields href and text. This way we support adding links to notifications while still making it possible to open a storybook and see all possible variants of notifications.

I think this applies here as well. Maybe we should instead add another prop, like:

headerIcon?: {
  Icon: React.ComponentType<IconProps>;
  ariaLabel: string // I assume we needed this for the icon?
}

@kiosion
Copy link
Contributor Author

kiosion commented Nov 7, 2024

@gzdunek hmm, yeah, that makes sense to me, will try something along those lines instead.

@kiosion kiosion closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants