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

refactor: extract icon scale adjustment logic into utility function #7935

Closed
wants to merge 3 commits into from

Conversation

alhridoy
Copy link

@alhridoy alhridoy commented Oct 4, 2023

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.

@alhridoy alhridoy requested a review from a team as a code owner October 4, 2023 05:56
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Looking great! Just a few tweaks and we can get this moving forward. 🚀

@jcfranco jcfranco changed the title Extract icon scale adjustment logic into utility function refactor: extract icon scale adjustment logic into utility function Oct 4, 2023
@jcfranco
Copy link
Member

jcfranco commented Oct 4, 2023

I've gone ahead and updated the PR title to follow conventional commits (per our contributing guidelines).

@alhridoy
Copy link
Author

alhridoy commented Oct 5, 2023

Hi @jcfranco Please check my new commits and let me know if everything is fine now. Thanks for your guidance.

Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Other than removing iconScaleAdjuster.spec.ts and resolving merge conflicts, this LGTM.

Once the last changes are made, we can take it from there to get screenshot tests to run. Thanks again! 🚀

@alhridoy
Copy link
Author

alhridoy commented Oct 7, 2023

Hi @jcfranco i am just checking if everything is ok on the latest changes. If I need to modify anything please let me know. Thanks

@jcfranco
Copy link
Member

jcfranco commented Oct 7, 2023

@alhridoy It looks like the PR still shows merge conflicts. Once these are resolved, we can take it from there. Thanks again for helping out with this!

@jcfranco
Copy link
Member

I went ahead and adopted this PR (#7973) to get both E2E and screenshot tests running. Once checks are ✅, we should be good to install for the October release.

@jcfranco
Copy link
Member

@alhridoy The adopted one (containing your original commits), yes.

jcfranco added a commit that referenced this pull request Oct 13, 2023
…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
Copy link
Member

Merged via #7973, closing!

Many thanks, @alhridoy!

@jcfranco jcfranco closed this Oct 13, 2023
@alhridoy
Copy link
Author

Hi @jcfranco Great! You welcome. Thanks a lot for guidances.

@alhridoy alhridoy deleted the my-con branch October 15, 2023 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants