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

FIX Regression issue UI URLSegment buttons #2886

Conversation

sabina-talipova
Copy link
Contributor

@GuySartorelli
Copy link
Member

Not doing a proper review, just looking at this in passing - but does this mean we can't have those icons on any buttons anywhere anymore? That seems like a regression that goes deeper than this PR would resolve.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Looks good. Tested in chrome and Firefox

@maxime-rainville maxime-rainville merged commit 34581be into silverstripe:5.1 Sep 17, 2023
12 checks passed
@maxime-rainville maxime-rainville deleted the pulls/5.1/fix-regression-ui-button branch September 17, 2023 23:28
@maxime-rainville
Copy link
Contributor

Not doing a proper review, just looking at this in passing - but does this mean we can't have those icons on any buttons anywhere anymore? That seems like a regression that goes deeper than this PR would resolve.

The icon still works fine on all the other buttons. Those split buttons were never meant to have icons and have some weird padding applied to them which was cause this strange behaviour.

Looking at the pattern lib, the you can still apply icons to them just fine.

https://silverstripe.github.io/silverstripe-pattern-lib/?path=/docs/admin-buttons-button--documentation

@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 17, 2023

The pattern library is for react components. The template (which is what this PR affects) is for the entwine implementation, I believe.

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