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

Create reusable util for adjusting internal icon scale used within a component #7765

Closed
1 of 2 tasks
jcfranco opened this issue Sep 18, 2023 · 9 comments
Closed
1 of 2 tasks

Create reusable util for adjusting internal icon scale used within a component #7765

jcfranco opened this issue Sep 18, 2023 · 9 comments
Assignees
@geospatialem @SkyeSeitz @ashetland @brittneytewks
Labels
4 - verified Issues that have been released and confirmed resolved. calcite-components Issues specific to the @esri/calcite-components package. estimate - 2 Small fix or update, may require updates to tests. good first issue Issues that can be worked on by contributors new to calcite-components. p - medium Issue is non core or affecting less that 60% of people using the library refactor Issues tied to code that needs to be significantly reworked.
Milestone
2023 October Prio...

Comments

@jcfranco
Copy link
Member

Description

Stems from #7755.

The logic to adjust internal icon scales (see

calcite-design-system/packages/calcite-components/src/components/alert/alert.tsx

Line 289 in d05c794

<calcite-icon icon="x" scale={this.scale === "l" ? "m" : "s"} />
) should be extracted into its own util and used across all components.

cc @driskull @ashetland @SkyeSeitz

Proposed Advantages

DRYer code by having component use this util to get the effective internal icon scale.

Which Component

  • accordion-item.tsx
  • action.tsx
  • alert.tsx
  • button.tsx
  • chip.tsx
  • color-picker-hex-input.tsx
  • color-picker.tsx
  • combobox-item.tsx
  • date-picker-month-header.tsx
  • dropdown-item.tsx
  • input-date-picker.tsx
  • input-number.tsx
  • input-text.tsx
  • input.tsx
  • modal.tsx
  • notice.tsx
  • pagination.tsx
  • popover.tsx
  • select.tsx
  • tab-title.tsx
  • table-header.tsx
  • table-row.tsx
  • time-picker.tsx
  • tree-item.tsx
  • XButton.tsx

Relevant Info

No response

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-react
@jcfranco jcfranco added refactor Issues tied to code that needs to be significantly reworked. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Sep 18, 2023
@github-actions github-actions bot added the calcite-components Issues specific to the @esri/calcite-components package. label Sep 18, 2023
@driskull
Copy link
Member

Sweet! this will be nice to have

@jcfranco jcfranco added the good first issue Issues that can be worked on by contributors new to calcite-components. label Sep 20, 2023
@alhridoy
Copy link

Hi @jcfranco I have reviewed the issue and I'd like to contribute by creating the reusable utility for adjusting internal
Icon scales. Is it alright if I take this up?

@driskull
Copy link
Member

@alhridoy sounds great. Feel free to take it on! Let us know if you have any questions.

@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. estimate - 2 Small fix or update, may require updates to tests. p - medium Issue is non core or affecting less that 60% of people using the library and removed 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Oct 4, 2023
@jcfranco
Copy link
Member Author

jcfranco commented Oct 4, 2023

@alhridoy Awesome! Thanks for taking this on. 🎉

I've assigned the issue, updated labels and assigned to the October release.

This was referenced Oct 4, 2023
Closed
Merged
@ashetland ashetland added the figma changes Issues that require additions or updates to the Figma UI Kit label Oct 11, 2023
jcfranco added a commit that referenced this issue Oct 13, 2023
@jcfranco
refactor: extract icon scale adjustment logic into utility function (#…
50f8732 
…7973)

# Adopted #7935
 
cc @alhridoy
 
---
 
**Related Issue:** #7765 

## Summary
This pull request extracts the logic for adjusting internal icon scales
into a separate utility function. This function is now used across all
components, making the code DRYer and easier to maintain.

## Changes
- Created a new utility function `adjustIconScale` in
`iconScaleAdjuster.ts`.
- Replaced inline logic for adjusting icon scales with calls to
`adjustIconScale` in all relevant components.

## Testing
- All unit tests pass.
- Manual testing in the browser confirms that icons still display
correctly at all scales.

This change stems from issue #7755 and is expected to make the codebase
easier to maintain by reducing repetition of this logic.

---------

Co-authored-by: Al-Iqram Elahee <hridoy@Al-Iqrams-MacBook-Pro.local>
@jcfranco jcfranco added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Oct 13, 2023
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@jcfranco
Copy link
Member Author

Thanks again @alhridoy for helping out with this! 🏆

@geospatialem
Copy link
Member

This looks fantastic! ✨

@jcfranco Follow-up on if we'd also want to include the XButton.tsx (housed in functional)? It looks like all the other list items are covered. 🙌🏻

calcite-design-system/packages/calcite-components/src/components/functional/XButton.tsx

Line 36 in f7ecc6c

<calcite-icon icon="x" scale={scale === "l" ? "m" : "s"} />

@jcfranco
Copy link
Member Author

Great catch! 🎣 Yes, we we should update it as well.

@geospatialem geospatialem mentioned this issue Oct 25, 2023
Merged
geospatialem added a commit that referenced this issue Oct 25, 2023
@geospatialem
refactor(xbutton): extract icon scale from utility function (#8056)
55132f4 
**Related Issue:**
#7765

## Summary
Finalizes up the above issue by tackling the `XButton.tsx` (functional
component) discussed in
#7765 (comment)
and
#7765 (comment).


cc: @jcfranco and related PR #7973
@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Oct 25, 2023
@geospatialem
Copy link
Member

Verified across components. 🎉

@SkyeSeitz SkyeSeitz removed the figma changes Issues that require additions or updates to the Figma UI Kit label Oct 27, 2023
@geospatialem geospatialem mentioned this issue Nov 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@geospatialem geospatialem

@SkyeSeitz SkyeSeitz

@ashetland ashetland

@brittneytewks brittneytewks

Labels
4 - verified Issues that have been released and confirmed resolved. calcite-components Issues specific to the @esri/calcite-components package. estimate - 2 Small fix or update, may require updates to tests. good first issue Issues that can be worked on by contributors new to calcite-components. p - medium Issue is non core or affecting less that 60% of people using the library refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Milestone
2023 October Priorities - Maintenance...
Development

No branches or pull requests
7 participants
@jcfranco @driskull @geospatialem @alhridoy @SkyeSeitz @ashetland @brittneytewks