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

Button type directly in class #763

Merged
merged 1 commit into from
Jan 12, 2024
Merged

Button type directly in class #763

merged 1 commit into from
Jan 12, 2024

Conversation

f-necas
Copy link
Collaborator

@f-necas f-necas commented Jan 11, 2024

Fix buttons not using their tailwind classes.

@f-necas f-necas requested a review from jahow January 11, 2024 18:19
Copy link
Contributor

github-actions bot commented Jan 11, 2024

Affected libs: ui-inputs, feature-dataviz, feature-record, feature-router, feature-editor, feature-search, feature-map, ui-elements, feature-catalog, ui-catalog, ui-search,
Affected apps: metadata-converter, metadata-editor, datahub, demo, webcomponents, search, map-viewer, datafeeder,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@f-necas f-necas marked this pull request as ready for review January 11, 2024 18:39
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

I don't really see the meaning of this change, it complexifies the button api.
Couldn't you adopt another approach ?

@f-necas
Copy link
Collaborator Author

f-necas commented Jan 12, 2024

@fgravin It's not really a big change, instead of primary we would have to write btn-primary.

It's just a small step to fix the display of not secondary buttons.

It's intended to get rid of this gn-ui-button either which will result in a <button type="button" class="btn-primary>"

But I couldn't really understand why in the get classList inside button component couldn't handle correctly btn-${type} but can handle ${type}

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Wait, how come this fixes the issue? edit: sorry, just saw the above messages. I think if we're going to modify every gn-ui-button usage then we might as well use the opportunity to get rid of this component altogether? But I would be very interested to know the underlying reason for this problem.

Revert "fix: button type directly in class"

This reverts commit e111a8e.

fix: button type directly in class
Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks!

@f-necas f-necas merged commit 3678ce5 into main Jan 12, 2024
8 checks passed
@jahow jahow deleted the btn-update branch January 12, 2024 15:10
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.

3 participants