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(button, fab): adjust padding on 'l' scale button to accommodate 'm' scale icon without change in height #5659

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Oct 27, 2022

Related Issue: #5245

Summary

Design adjustment on #5521. The height of the large component changed when a new medium (24px) icon was introduced. This PR will meet the design recommendation to maintain the 44px height with or without an icon, accounting for both the default solid appearance with a border and transparent without. Every other appearance option also has a border. Also includes small adjustments for other scales.

…a medium size icon without a change in height
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Oct 27, 2022
@Elijbet Elijbet marked this pull request as ready for review October 28, 2022 18:37
@Elijbet Elijbet requested a review from a team as a code owner October 28, 2022 18:37
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Will screener pick up these changes somewhere? We need a visual test here so it doesn't break :)

src/components/button/button.scss Outdated Show resolved Hide resolved
@Elijbet Elijbet marked this pull request as draft October 28, 2022 20:31
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Review feedbackz! ʘ‿ʘ

src/components/button/button.scss Outdated Show resolved Hide resolved
src/components/button/button.scss Outdated Show resolved Hide resolved
src/components/button/button.scss Outdated Show resolved Hide resolved
src/components/button/button.tsx Outdated Show resolved Hide resolved
src/components/button/button.stories.ts Show resolved Hide resolved
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 3, 2022
@Elijbet Elijbet marked this pull request as ready for review November 3, 2022 17:35
@eriklharper eriklharper added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 7, 2022
…with-larger-icon' of github.com:Esri/calcite-components into elijbet/5245-l-scale-button/fab-retain-existing-height-with-larger-icon
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 8, 2022
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 8, 2022
@benelan benelan added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 8, 2022
@Elijbet Elijbet removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 8, 2022
@github-actions github-actions bot added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 8, 2022
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 9, 2022
Copy link
Member

@driskull driskull 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 @Elijbet

src/components/button/button.scss Show resolved Hide resolved
src/components/button/button.scss Show resolved Hide resolved
@benelan benelan added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 9, 2022
@Elijbet Elijbet merged commit d68d95c into master Nov 9, 2022
@Elijbet Elijbet deleted the elijbet/5245-l-scale-button/fab-retain-existing-height-with-larger-icon branch November 9, 2022 23:55
benelan added a commit that referenced this pull request Nov 11, 2022
* master: (32 commits)
  1.0.0-next.627
  build(deps): bump semver-regex and screener-storybook (#5741)
  build(deps): bump stylelint-config-recommended-scss from 7.0.0 to 8.0.0 (#5551)
  build(deps): bump parse-path and @storybook/storybook-deployer (#5692)
  build(deps): bump tailwindcss from 3.1.8 to 3.2.2 (#5708)
  ci(pr-bot): check user login for dependabot instead of actor (#5740)
  build(deps): bump postcss from 8.4.17 to 8.4.18 (#5508)
  build(deps): bump concurrently from 7.4.0 to 7.5.0 (#5550)
  build(deps): bump loader-utils from 1.4.0 to 1.4.1 (#5704)
  build(deps): bump @typescript-eslint/parser from 5.40.1 to 5.42.1 (#5707)
  test(value-list): Skip unstable test. (#5738)
  ci(pr-bot): skip assignment for dependabot PRs (#5737)
  chore(t9manifest): Fix newline at end of file. (#5718)
  fix(flow-item): Position back tooltip above (#5688)
  feat(popover): Escape key should close open popovers. (#5726)
  docs: update component READMEs (#5545)
  fix(value-list-item): Prevent scrolling when space is pressed on drag button (#5709)
  ci(output targets): move patches to postbuild to include them in CCR (#5730)
  1.0.0-next.626
  fix(button, fab): adjust padding on 'l' scale button to accommodate 'm' scale icon without change in height (#5659)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants