Skip to content

Conversation

@jkachel
Copy link
Contributor

@jkachel jkachel commented Jun 24, 2024

What are the relevant tickets?

Closes mitodl/hq#4589
Closes mitodl/hq#4104

Description (What does it do?)

Swaps out icons with Remixicons where noted in this spreadsheet.

In addition, this updates the TopicsIcon component to take either a string or a React element - the string functionality was left in there in case we need it for something that needs an icon that's not a Remixicon, so not opposed to just making it take a React element.

Screenshots (if appropriate):

Topics icons:
topics

Departments icons:

depts-1
depts-2
depts-3
depts-4

Side nav drawer:

sidebar

Profile page:

profile-opts

Course card icons (some of them - I'm not real sure how to force it to display the Number of Courses one):

course-card-icons-2
Screenshot 2024-06-27 at 10 33 40 AM

Lock chips:

lock-icons

How can this be tested?

View the site. Compare and contrast the icons used against the designs in Figma and the icons doc.

I removed a bunch of now-unused SVG icons as well so those shouldn't be in the build either.

Notes

There was a least one place where the icons we were using were 22x22px (in the sidebar nav) - I did not adjust the new icons to be 22x22 and left them at the default 24x24px because 24 is cleanly divisible by 8.

@jkachel jkachel added product:mit-open Issues related to the MIT Open product Work in Progress labels Jun 24, 2024
@jkachel jkachel changed the title Jkachel/4589 4104 update all icons to remixicon Updates icons to use Remixicons where they don't already Jun 24, 2024
@jkachel jkachel force-pushed the jkachel/4589-4104-update-all-icons-to-remixicon branch from 6feaaf1 to 047372c Compare June 27, 2024 15:28
@jkachel jkachel added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Jun 27, 2024
@jkachel jkachel marked this pull request as ready for review June 27, 2024 15:42
@shanbady shanbady self-requested a review June 28, 2024 13:19
@shanbady shanbady self-assigned this Jun 28, 2024
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

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

It seems like we still have a bunch of references to mui icons like SettingsIcon in channelmenu.tsx Dragmenu in CardTemplate.tsx, BookmarkBorderIcon in LearningResourceCard. Are we leaving those in place for some reason?

@jkachel
Copy link
Contributor Author

jkachel commented Jun 28, 2024

I updated the icons in the LearningResourceCard. I couldn't find the other two in Figma and they're not in the Google doc (linked here) so I wasn't sure what to change those to - @steven-hatch or @mbilalmughal may have some input here?

Edit: I changed SettingsIcon to RiSettingsLine and DragIndicatorIcon to RiDraggable - these seemed reasonable but they may need to change. (Though there wasn't a dotty draggable icon other than RiDraggable.)

@jkachel jkachel requested a review from shanbady June 28, 2024 19:12
…ates

The Mui icons seem to do that by default but the Remixicons don't so the test was failing.
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

Searching for @mui/icons-material, there seem to many remaining material icons: 34 matches in .tsx, .ts files, 13 in storybook files and 21 elsewhere.

  • 13 in storybook files ... These should be easy to replace. They don't really matter. They are just there to use an icon. What icon is irrelevant.
  • 21 elsewhere

The "21 elsewhere" are in files

mit-open/src/page-components/
    /ChannelDetails/ChannelDetails.tsx
    /Dialogs/AddToListDialog.tsx
    /LearningResourceCardTemplate/LearningResourceCardTemplate.tsx
    /SearchDisplay/SearchInput.tsx
    /SearchSubscriptionToggle/SearchSubscriptionToggle.tsx

mit-open/src/pages/
    /HomePage/SearchInput.tsx
    /LearningPathListingPage/LearningPathListingPage.tsx
    /ListDetailsPage/ListDetailsPage.tsx
    /UserListListingPage/UserListListingPage.tsx


ol-widgets/src/components/
    /Widget.tsx
    /editing/WidgetsListEditable.tsx

Having looked at them a bit, the replacements I would make are:

MUI                     remixicon
OpenInNew               external-link-line
Add                     add-line
CalendarTodayIcon       calendar-line               note: not user facing right now
ClearIcon               close-line
ExpandMoreSharp         arrow-down-s-line
MailOutline             mail-line
Edit                    edit-line
MoreVert                more-2-line
Delete                  delete-bin-line
SwapVert                arrow-up-down-line
ExpandMore              arrow-down-s-line
ExpandLess              arrow-up-s-line
DragHandle              draggable** (not user facing)

The mui icon DragHandle doesn't have a great replacement in remixicon. But it's (a) only in a staff interface, and (b) that staff interface isn't even used right now (widget sorting). So I don't think we should worry about the replacement being sub optimal.

@shanbady
Copy link
Contributor

shanbady commented Jul 2, 2024

@ChristopherChudzicki I think this can be looked at again. I should have all the material icons replaced

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

The app looks good. I'm still seeing a few references to @mui/icons-material... could you remove these?

Screenshot 2024-07-02 at 4 25 20 PM

I don't think this needs a re-review, but I would recommend doing some sort of "Find all" for @mui/icons-material to double check before merging.

@shanbady shanbady dismissed their stale review July 2, 2024 20:39

dismissing self review

@shanbady shanbady merged commit 34a0b04 into main Jul 2, 2024
@shanbady shanbady deleted the jkachel/4589-4104-update-all-icons-to-remixicon branch July 2, 2024 20:40
This was referenced Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review product:mit-open Issues related to the MIT Open product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants