feat(button): add circular shape as round #29161
Conversation
Co-authored-by: Brandy Carney <[email protected]>
| ::slotted(ion-icon[slot="icon-only"]) { | ||
| font-size: 1.8em; | ||
| } |
There was a problem hiding this comment.
Removed since the font size is being overwritten through their respective mode files.
thetaPC
requested review from
averyjohnston,
brandyscarney,
liamdebeasi,
sean-perkins and
a team
as code owners
There was a problem hiding this comment.
I think we are going to have to implement this using min-width or aspect-ratio instead of adjusting the padding because the outline icons are wider than they are tall due to the borders.
|
Can you split the screenshots to a separate PR? I can't review the files without it freezing. |
There was a problem hiding this comment.
This PR seems to be doing multiple things:
- Adds circle shape appearance to buttons.
- Updates the sizing of all buttons, creating many more diffs than specific to the feature we are adding.
It is possible I've been out of these conversations, but the recommended sizing was specific to circular buttons. Those haven't been reviewed or approved for buttons at whole.
We can take this to internal comms 👍
| @@ -295,6 +305,10 @@ export class Button implements ComponentInterface, AnchorInterface, ButtonInterf | |||
| this.ionBlur.emit(); | |||
| }; | |||
|
|
|||
| private slotChanged = () => { | |||
| this.isCircle = this.hasIconOnly; | |||
There was a problem hiding this comment.
~ We should explain that we are using a state variable to trigger a re-render of the Stencil component when the slotted content changes, so that has-icon-only class is correctly applied.
I think moving that documentation here to enable the earlier documentation to be more focused on what isCircle is - a variable to track if the button should render as a circle shape or not.
There was a problem hiding this comment.
The button sizes do not appear to accurate to the PR description and design document. Can we double check those?
I checked locally against: http://localhost:3333/src/components/button/test/round on iOS all the sizes are ~2px smaller than expected.
There was a problem hiding this comment.
The feature achieves the core requirements we aimed to accomplish and looks great, great work!
One question/potentially a follow up task we put in the backlog & refine:
The min clamping is great, we may want to consider what happens when the font size is too large:
With the far right image, I've set the font size to 100px.
We hadn't considered this in the original design and I think it is a less common concern, so I'm fine with tackling that later.
|
@sean-perkins I'll open a new PR for the font sizes. It should be up soon. |
There was a problem hiding this comment.
Is this the correct value for the large icon only button? The icon is set to 1em which is smaller than the small and default button.
There was a problem hiding this comment.
Looking into this now
There was a problem hiding this comment.
@brandyscarney this is correct. The large button has a font-size of 20px so the 1em is using that as the base. While the default has a font-size of 16px and small is 13px.
Issue number: internal
What is the current behavior?
The
roundshape with only an icon will display a smaller value for the border radius.What is the new behavior?
The
roundshape with only an icon will display as a circle.mdandios.ion-button) since Figma did not provide them.shape="round".shape="round".Does this introduce a breaking change?
N/A
Other information
Screenshots by components: