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.

Sign up for GitHub

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

Jump to bottom

fix(pie-icons-webc): DSW-1539 Remove :host-context usage for icon sizing #1107

Merged
merged 16 commits into from
Dec 20, 2023
Merged

fix(pie-icons-webc): DSW-1539 Remove :host-context usage for icon sizing #1107

merged 16 commits into from
Dec 20, 2023

Conversation

kevinrodrigues
Copy link
Contributor

@kevinrodrigues kevinrodrigues commented Dec 13, 2023 •
edited
Loading

Describe your changes (can list changeset entries if preferable)

Before:
Screenshot 2023-12-14 at 11 52 07

After:

Screenshot 2023-12-14 at 11 49 32

Author Checklist (complete before requesting a review)

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests
  • If it is a PIE Docs change, I have reviewed the Docs site preview
  • If it is a component change, I have reviewed the Storybook preview
  • If there are visual test updates, I have reviewed them properly before approving

Reviewer checklists (complete before approving)

Reviewer 1

  • If it is a PIE Docs change, I have reviewed the PR preview
  • If there are visual test updates, I have reviewed them

Reviewer 2

  • If it is a PIE Docs change, I have reviewed the PR preview
  • If there are visual test updates, I have reviewed them

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 13, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 3f77cd7

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

This PR includes changesets to release 8 packages
Name Type
@justeattakeaway/pie-icons-webc Minor
wc-vanilla Patch
pie-storybook Patch
@justeattakeaway/pie-icon-button Patch
@justeattakeaway/pie-modal Patch
@justeattakeaway/pie-switch Patch
@justeattakeaway/pie-cookie-banner Patch
wc-next10 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

@kevinrodrigues
Copy link
Contributor Author

/snapit

@github-actions github-actions bot added the tools label Dec 13, 2023
@kevinrodrigues
Copy link
Contributor Author

/snapit

1 similar comment
@kevinrodrigues
Copy link
Contributor Author

/snapit

@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

🫰✨ Thanks @kevinrodrigues! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]

@kevinrodrigues kevinrodrigues added DO NOT MERGE work-in-progress This pull request is still a work in progress and may not be ready for review labels Dec 13, 2023
@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

The build failed, please see the logs.

@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

The build failed, please see the logs.

@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

🫰✨ Thanks @kevinrodrigues! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]

@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

🫰✨ Thanks @kevinrodrigues! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]

@kevinrodrigues kevinrodrigues removed work-in-progress This pull request is still a work in progress and may not be ready for review DO NOT MERGE labels Dec 14, 2023
fernandofranca
fernandofranca previously approved these changes Dec 19, 2023
View reviewed changes
@kevinrodrigues kevinrodrigues changed the title fix(pie-icons-webc): DSW-1539 atempt to resolve icon sizing issue fix(pie-icons-webc): DSW-1539 resolve icon sizing issue Dec 19, 2023
@kevinrodrigues @ashleynolan
Update .changeset/few-gifts-shop.md
babe44e 
Co-authored-by: Ashley Watson-Nolan <[email protected]>
@kevinrodrigues kevinrodrigues changed the title fix(pie-icons-webc): DSW-1539 resolve icon sizing issue fix(pie-icons-webc): DSW-1539 Remove :host-context usage for icon sizing Dec 19, 2023
@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

The build failed, please see the logs or take a look at the Workflow Tooling wiki page to make sure your PR meets the requirements.

@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

The build failed, please see the logs or take a look at the Workflow Tooling wiki page to make sure your PR meets the requirements.

@kevinrodrigues
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

🫰✨ Thanks @kevinrodrigues! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]
yarn add @justeattakeaway/[email protected]

@kevinrodrigues kevinrodrigues marked this pull request as ready for review December 20, 2023 07:58
@kevinrodrigues kevinrodrigues requested review from a team as code owners December 20, 2023 07:58
@kevinrodrigues kevinrodrigues merged commit 270b467 into main Dec 20, 2023
61 checks passed
@kevinrodrigues kevinrodrigues deleted the dsw-1539-icon-size-bug branch December 20, 2023 08:31
@pie-design-system-bot pie-design-system-bot mentioned this pull request Dec 20, 2023
Merged
dandel10n
dandel10n reviewed Dec 21, 2023
View reviewed changes
packages/tools/pie-icons-webc/build.js
display: block;
width: var(--btn-icon-size);
height: var(--btn-icon-size);
:host svg {
Copy link
Contributor

@dandel10n dandel10n Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This change will mess up icon size for all our icons making the svg container 24px X 24px. Here is a Link component with an icon as an example:
Screenshot 2023-12-21 at 10 49 26

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a PR for the fix: #1127

@jamieomaguire
Copy link
Contributor

@kevinrodrigues @ashleynolan was this visually tested in Aperture NextJS and Vanilla before merging?

@ashleynolan
Copy link
Contributor

@jamieomaguire Yeh, it was tested – I think it just got missed as it's a subtle sizing change (but it is showing up the issue in the Version Package PR now too)

I'm a bit confused as to why the link isn't being flagged too – unless we don't have visual tests for links with icons.

@dandel10n is spinning up a PR with the fix for this, so that'll sort it out shortly too 👍

@ashleynolan
Copy link
Contributor

ashleynolan commented Dec 21, 2023 •
edited
Loading

@jamieomaguire @dandel10n Just to follow up on this, the reason the pie-link visual tests haven't also broke is because they don't use the pie-webc icons in the visual tests. They just use a hardcoded SVG by the looks of it.

@pie-design-system-bot pie-design-system-bot mentioned this pull request Jan 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamieomaguire jamieomaguire jamieomaguire left review comments

@dandel10n dandel10n dandel10n left review comments

@fernandofranca fernandofranca fernandofranca approved these changes

@ashleynolan ashleynolan ashleynolan approved these changes

Assignees
No one assigned
Labels
components pie-button pie-icon-button tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kevinrodrigues @pie-design-system-bot @jamieomaguire @ashleynolan @fernandofranca @dandel10n