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

TextLink: Honor className provided via props #88273

Closed
wants to merge 1 commit into from

Conversation

domasx2
Copy link
Contributor

@domasx2 domasx2 commented May 24, 2024

What is this feature?

TextLink accepts className prop but does not use it. This PR makes TextLink honor the className prop.

Why do we need this feature?

Have a use case to add a class name to a TextLink to adjust positioning 🙇

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.

@domasx2 domasx2 added area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes internal for issues made by grafanistas labels May 24, 2024
@domasx2 domasx2 requested a review from a team as a code owner May 24, 2024 09:19
@domasx2 domasx2 requested review from joshhunt and L-M-K-B and removed request for a team May 24, 2024 09:19
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone May 24, 2024
Copy link
Contributor

@L-M-K-B L-M-K-B left a comment

Choose a reason for hiding this comment

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

Hi Domas,
When we, FE Platform Squad, created this component we agreed to provide a component with default styling which isn't supposed to be overwritten (see storybook). That's why accepting the classname prop doesn't seem to be a good option, but it would be great if you could add the changes required for positioning the TextLink in the right way.

@domasx2
Copy link
Contributor Author

domasx2 commented May 24, 2024

Hey!

Thanks for the explanation

When we, FE Platform Squad, created this component we agreed to provide a component with default styling which isn't supposed to be overwritten (see storybook). That's why accepting the classname prop doesn't seem to be a good option

I don't see why default styling and allowing a class name should be mutually exclusive 🤔 There's use cases for adding a class name that have nothing to do with overriding default styling. Nor preventing adding a class name prevents you from changing the default styling via other means. It's just a minor inconvenience. Eg can use style attribute instead or a wrapper element

but it would be great if you could add the changes required for positioning the TextLink in the right way.

My use case is very specific, imho not worth generalizing. I can just work around this using a wrapper

In any case, className prop being availabe and not having an effect is misleading. If this PR is not acceptable may be worhtwhile to at least exclude it from props then

@diegommm diegommm modified the milestones: 11.1.x, 11.2.x Jun 14, 2024
@diegommm
Copy link
Contributor

Hi! Greetings from the Release Guild. Just letting you know I'm updating your PR's milestone to 11.2.x. If you need to see your changes in a previous version, please consider adding a backport label.

Thank you!

@domasx2 domasx2 closed this Jun 21, 2024
@grafana-delivery-bot grafana-delivery-bot bot removed this from the 11.2.x milestone Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend internal for issues made by grafanistas no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants