Button v2: Fix icon positions for block buttons by siddharthkp · Pull Request #1733 · primer/react

siddharthkp · 2021-12-14T17:45:57Z

With a block button, the icon and text alignment is a bit strange

Screenshots

Please provide before/after screenshots for any visual changes

The chromatic tests (see checks below) show that there are no visual regressions :)

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2021-12-14T17:45:58Z

🦋 Changeset detected

Latest commit: 601226b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2021-12-14T17:49:20Z

size-limit report 📦

Path	Size
dist/browser.esm.js	57.54 KB (+0.07% 🔺)
dist/browser.umd.js	57.83 KB (+0.06% 🔺)

pksjce · 2021-12-15T05:41:16Z

Interesting..
So, if only the trailingIcon exists, then we get the text back to the middle?

pksjce · 2021-12-15T05:48:01Z

 gridArea: 'trailingIcon'
+ },
+ '[data-component="leadingIcon"] + [data-component="text"]': {
+ textAlign: 'left'


Not sure if this leftAlign is required tbh.

fair, how do we make it easier for the user to override this with left align?
this would be the override, not sure if requires knowing the internal details?
<ButtonleadingIcon={EyeIcon}trailingIcon={TriangleDownIcon}sx={{'[data-component="text"]': {textAlign: 'left'}}}> Watch </Button>

True :( This is where composite components would be handy. Maybe such an override usecase means we want to open up the API?

If we go with the center-aligned approach I recommended, we can get rid of this style block.

siddharthkp · 2021-12-15T10:42:06Z

Interesting.. So, if only the trailingIcon exists, then we get the text back to the middle?

Sorry, my PR was a bit rushed. This is what I'm proposing: (will make changes to the PR if we like the proposal)

cc @ashygee

Figma: Primer Web #sid/block-button-layout

mperrotti

I think left-aligning the leadingVisual and label when we have a trailingVisual is a bit strange. It will especially look weird if we have two stacked full-width buttons where 1 has left-aligned text, and the other has center-aligned text.

I think the "current" column of the mock you posted is what we want.

Curious to hear what @ashygee thinks

mperrotti · 2021-12-16T21:32:43Z

 ...getBaseStyles(theme),
 display: 'grid',
 gridTemplateAreas: '"leadingIcon text trailingIcon"',
+ gridTemplateColumns: 'min-content 1fr min-content',


If we decide to go with the center-aligned approach I recommended, we can remove this style and just add justify-content: center;

siddharthkp · 2021-12-17T11:04:13Z

For context, this is the pattern I'm chasing:

This is what the code looks like for positioning it correctly (ignoring the hover state for now):

To be able to override it correctly, you have to know some implementation details about it, which feels wrong :(

<NewButtonvariant="invisible"trailingIcon={GearIcon}sx={{width: "100%",justifyContent: "space-between",gridTemplateAreas: '"text trailingIcon"'}}>

pksjce · 2021-12-21T23:17:45Z

@mperrotti - If there is a product use case then I think we should work on supporting the new mocks.
@siddharthkp - Would a boolean prop for block which takes care of this situation better? Though I'm still not sure how the alignment will work.

pksjce · 2021-12-22T04:18:38Z

Since @ashygee is on holidays, I was wondering if @vdepizzol could help us with this?

pksjce · 2021-12-22T04:27:43Z

This is what primer-viewcomponents looks like with block button and visuals.

And in primer-css

Center-align seems standard in our system.

siddharthkp · 2021-12-22T10:51:15Z

Context:

If there is a product use case then I think we should work on supporting the new mocks.

only issues-app for now I think, but feels like a generic use case, we can also decide we want this alignment to be an override in the product instead of supporting it out of the box.

@siddharthkp - Would a boolean prop for block which takes care of this situation better? Though I'm still not sure how the alignment will work.

That could work!

Since @ashygee is on holidays, I was wondering if @vdepizzol could help us with this?

That would be nice. This can also wait, it's not a december problem :)

* add template columns and text align for text * only align left if there's a leading Icon * add changeset * add warning for multiple selection with DropdownMenu * wip: stories * use leadingIcon with #1733 * add form to story example * use menuitemradio + aria-checked * fix types for selectionProperty * it's not a property, it's an attribute * placeholder in story * story with placeholder * oops, don't want forms to submit! * selected milestone can be undefined! * update snapshots * Add story for controlled menu and external anchor * add tests! * export DropdownMenu2 as draft * default to selectionVariant single for DropdownMenu * dont need axe anymore * update octicons for docs * add docs for dropdown menu v2 * remove changes from #1733 * update snapshots for removed button * link to button2 docs * add changeset * lol everybody makes mistakes * oof! * Use active voice in component description Co-authored-by: Rez <rezrah@github.com> * update snapshosts with main * just Props Co-authored-by: Rez <rezrah@github.com> * Use PropsTable * simplify code example a bit * make controlled state more clear * keep lockfile version 1 * remove unrelated change Co-authored-by: Rez <rezrah@github.com>

mperrotti · 2022-01-25T21:11:44Z

This came up during a Primer design pattern working session, and I like how @langermank decided to handle it

Storybook preview deployment link: https://primer-css-goebhg4xi-primer.vercel.app/css/storybook?path=/story/explorations--example-sheet
WIP PR: primer/css#1874

siddharthkp · 2022-02-23T11:47:14Z

This isn't the direction in which we're going ✌️

siddharthkp requested review from a team and rezrah December 14, 2021 17:45

siddharthkp requested review from pksjce and removed request for rezrah December 14, 2021 17:46

siddharthkp self-assigned this Dec 14, 2021

siddharthkp added the react label Dec 14, 2021

siddharthkp changed the title ~~Button2: Fix positions for block buttons~~Button2: Fix icon positions for block buttonsDec 14, 2021

siddharthkp changed the title ~~Button2: Fix icon positions for block buttons~~Button v2: Fix icon positions for block buttonsDec 14, 2021

siddharthkp added a commit that referenced this pull request Dec 14, 2021
use leadingIcon with#1733
616b36e

pksjce reviewed Dec 15, 2021
View reviewed changes

siddharthkpand others added 5 commits December 15, 2021 16:49

add template columns and text align for text
cdb29e5

only align left if there's a leading Icon
e56eb42

add changeset
206a9e2

update button snapshot
e562c65

Add a few more permutations of the block with icons example
601226b

pksjce force-pushed the siddharth/newbutton-gird-width branch from 46ab6f3 to 601226bCompare December 15, 2021 05:49

pksjce requested a review from langermank December 16, 2021 00:36

colebemis requested a review from mperrotti December 16, 2021 00:51

mperrotti reviewed Dec 16, 2021
View reviewed changes

siddharthkp added a commit that referenced this pull request Dec 21, 2021
remove changes from#1733
aeb4b29

pksjce mentioned this pull request Jan 14, 2022
WIP: Button/Link API exploration primer/css#1874
Closed
7 tasks

siddharthkp marked this pull request as draft January 26, 2022 11:36

siddharthkp closed this Feb 23, 2022

joshblack deleted the siddharth/newbutton-gird-width branch January 19, 2023 16:47

Uh oh!

Conversation

siddharthkp commented Dec 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Merge checklist

Uh oh!

changeset-botBot commented Dec 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actionsBot commented Dec 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

pksjce commented Dec 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pksjceDec 15, 2021

Choose a reason for hiding this comment

Uh oh!

siddharthkpDec 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pksjceDec 16, 2021

Choose a reason for hiding this comment

Uh oh!

mperrottiDec 16, 2021

Choose a reason for hiding this comment

Uh oh!

siddharthkp commented Dec 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mperrotti left a comment

Choose a reason for hiding this comment

Uh oh!

mperrottiDec 16, 2021

Choose a reason for hiding this comment

Uh oh!

siddharthkp commented Dec 17, 2021

Uh oh!

pksjce commented Dec 21, 2021

Uh oh!

pksjce commented Dec 22, 2021

Uh oh!

pksjce commented Dec 22, 2021

Uh oh!

siddharthkp commented Dec 22, 2021

Uh oh!

mperrotti commented Jan 25, 2022

Uh oh!

siddharthkp commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

siddharthkp commented Dec 14, 2021•
edited
Loading

changeset-botBot commented Dec 14, 2021•
edited
Loading

github-actionsBot commented Dec 14, 2021•
edited
Loading

pksjce commented Dec 15, 2021•
edited
Loading

siddharthkpDec 15, 2021•
edited
Loading

siddharthkp commented Dec 15, 2021•
edited
Loading