Skip to content

feat(ui-rating): rework Rating#2461

Open
ToMESSKa wants to merge 1 commit intomasterfrom
INSTUI-4806-rating-rework-multi
Open

feat(ui-rating): rework Rating#2461
ToMESSKa wants to merge 1 commit intomasterfrom
INSTUI-4806-rating-rework-multi

Conversation

@ToMESSKa
Copy link
Contributor

@ToMESSKa ToMESSKa commented Mar 17, 2026 •
edited
Loading

INSTUI-4806

ISSUE:

  • Rating needs to be migrated to the new theming system

TEST PLAN:

  • check if the component works the same as before in v1
  • check if the components and the examples work in v2 (especially the dark theme)
  • check if all necessary the imports are renamed to latest in v2
  • check if the test files are removed from the v1
  • check if the right imports are used in exports/b.ts and exports/a.ts
  • check if the package.json uses the correct versioning
  • check if the renamed or removed tokes are documented in the upgrade guide
  • check if the index.ts files use withStyle instead of withStyleLegacy
  • check if the v2 component uses all the tokens

see this link to see the rework: https://instructure.design/pr-preview/pr-2461/v11_7/Rating

@ToMESSKa ToMESSKa self-assigned this Mar 17, 2026
@github-actions
Copy link

github-actions bot commented Mar 17, 2026 •
edited
Loading

PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://instructure.design/pr-preview/pr-2461/

Built to branch gh-pages at 2026-03-24 12:35 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@ToMESSKa ToMESSKa force-pushed the INSTUI-4806-rating-rework-multi branch from 8323c60 to 099e8ef Compare March 17, 2026 12:30
ToMESSKa
ToMESSKa commented Mar 17, 2026
View reviewed changes
packages/ui-icons/src/custom/wrapCustomIcon.tsx
Comment on lines 103 to +104
ref: handleElementRef,
name: iconName,
Copy link
Contributor Author

@ToMESSKa ToMESSKa Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The svg had no name prop so the test could not find the custom filled star icon, so I added this.

@ToMESSKa ToMESSKa force-pushed the INSTUI-4806-rating-rework-multi branch from 099e8ef to 133042f Compare March 17, 2026 12:43
matyasf
matyasf reviewed Mar 18, 2026
View reviewed changes
packages/ui-rating/src/RatingIcon/v2/styles.ts
* @return {Object} The final style object, which will be used in the component
*/
const generateStyle = (
componentTheme: NewComponentTypes['RatingIcon'],
Copy link
Collaborator

@matyasf matyasf Mar 18, 2026

Choose a reason for hiding this comment

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

You are not using smallIconFontSize, mediumIconFontSize, largeIconFontSize from the theme, I guess these tokens are not needed?

matyasf
matyasf approved these changes Mar 18, 2026
View reviewed changes
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

There are some unused tokens, but I think this is a design issue. If yes, can you tell them? Thanks!

@joyenjoyer
Copy link
Contributor

joyenjoyer commented Mar 19, 2026

check if the renamed or removed tokes are documented in the upgrade guide

I see no changes in the upgrade guide. Why?

@ToMESSKa ToMESSKa force-pushed the INSTUI-4806-rating-rework-multi branch from 133042f to e57700d Compare March 24, 2026 12:30
@ToMESSKa
Copy link
Contributor Author

ToMESSKa commented Mar 24, 2026

@hajnaldo smallIconFontSize, mediumIconFontSize, largeIconFontSize can you please remove these tokens? The icon size is now controlled through the size prop now.

@ToMESSKa
Copy link
Contributor Author

ToMESSKa commented Mar 24, 2026

check if the renamed or removed tokes are documented in the upgrade guide

I see no changes in the upgrade guide. Why?

@joyenjoyer Thanks, I updated the upgrade guide withe the deleted tokens

joyenjoyer
joyenjoyer approved these changes Mar 25, 2026
View reviewed changes
Copy link
Contributor

@joyenjoyer joyenjoyer 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 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matyasf matyasf matyasf approved these changes

@joyenjoyer joyenjoyer joyenjoyer approved these changes

@hajnaldo hajnaldo Awaiting requested review from hajnaldo

Assignees

@ToMESSKa ToMESSKa

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ToMESSKa @joyenjoyer @matyasf