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

Upgrade Google Material Icons #251

Merged
merged 8 commits into from
Jul 12, 2021

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Jul 2, 2021

Description

This PR upgrades @material-icons/svg package (Google Material Icons fork) from 0.0.1 to its latest version 1.0.12 so that we have access to some new icons that are needed for new products features and are missing in the older version (I will expose them in a different PR). It also contains updated precompiled Vue svg icons files and reStructuredText replacement strings.

Before/after of KDS public icons

1. The filled copy/duplicate and the outlined copyToClipboard is merged into one outlined copy (CHANGELOG)

Before After
Screenshot from 2021-07-02 13-35-03 Screenshot from 2021-07-02 13-35-15
Screenshot from 2021-07-02 13-35-09 Screenshot from 2021-07-02 13-35-15

The former filled copy/duplicate icon wasn't filled anymore after the @material-icons/svg package upgrade and looked exactly the same as the outlined copyToClipboard icon, which means that the outlined icon newly represented all three aliases. @jtamiace confirmed that it's fine to deprecate the filled icon in KDS public icons set too. In addition to that, we will only keep copy alias for this icon which is the most commonly used alias in our products.

@radinamatic This also means that duplicate and copyToClipboard icons won't be available in reStructuredText replacement strings anymore and should be replaced by copy icon.

2. domain and facility icons have been merged into one icon with domain and facility aliases

It seems that previously, there were some, hard to perceive, differences in svgs that are not there anymore, so the same icon is indeed the same icon now :)

Before After
Screenshot from 2021-07-02 14-08-32 Screenshot from 2021-07-02 14-08-48
Screenshot from 2021-07-02 14-08-40 Screenshot from 2021-07-02 14-08-48

3. Minor updates in svg content of email, settings, indeterminateCheck, lesson, and hint icons

I haven't noticed any visual differences for these.

Updated KDS icons documentation page

kds-icons
:

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

MisRob added 4 commits July 2, 2021 12:58
After material icons upgrade, the former filled copy/duplicate icon
is not filled anymore and looks exactly the same as the same but outlined
copyToClipboard icon, which means that the outlined icon
now represents all three aliases. We agreed to only use `copy`
alias for the icon which is the most commonly alias used
in our products.
@MisRob
Copy link
Member Author

MisRob commented Jul 2, 2021

@indirectlylit Where would be a good place to save a note about "1. The filled copy/duplicate and the outlined copyToClipboard is merged into one outlined copy"? I assume that at the end of the day this will come to the GitHub Releases page and KDS version history page but I am not sure where to log updates before we have a new release.

@indirectlylit
Copy link
Contributor

domain and facility icons have been merged into one icon with domain and facility aliases

What kind of 'domain' is this referring to? This seems like a mistake, if we're using the facility icon for other reasons.

@indirectlylit
Copy link
Contributor

Where would be a good place to save a note about "1. The filled copy/duplicate and the outlined copyToClipboard is merged into one outlined copy"?

please add these notes to the changelog page:

@MisRob
Copy link
Member Author

MisRob commented Jul 7, 2021

What kind of 'domain' is this referring to? This seems like a mistake, if we're using the facility icon for other reasons.

@indirectlylit I don't know how we use all icons, but taking into account that we need to define them in iconsDefinitions file manually, my guess would be that it's been exposed on purpose.

@MisRob
Copy link
Member Author

MisRob commented Jul 7, 2021

please add these notes to the changelog page:

@indirectlylit
Okay, where should it go? In the first section 0.2.1?

@indirectlylit
Copy link
Contributor

I don't know how we use all icons, but taking into account that we need to define them in iconsDefinitions file manually, my guess would be that it's been exposed on purpose.

I don't think that it was exposed on purpose with the token domain, and I believe this token is no longer used anywhere.

In Material Icons, 'business' and 'domain' are aliases to the same icon:

image

The domain version in icon definitions has the comment //FacilityTopNav:

//FacilityTopNav
save: { icon: require('./precompiled-icons/material-icons/save/baseline.vue').default },
domain: { icon: require('./precompiled-icons/material-icons/domain/baseline.vue').default },

However, the token is not used in there anymore. It was removed when we transitioned this icon from representing 'classes' to its current meaning of 'facility':

learningequality/kolibri@219e74e

Long story short, I think we can safely drop the domain alias and always just use facility.

Okay, where should it go? In the first section 0.2.1?

That's fine. I need to work with @sairina at some point to get the changelog fully up-to-date with all the changes we haven't logged.

@MisRob
Copy link
Member Author

MisRob commented Jul 8, 2021

Thanks @indirectlylit , I removed domain alias and updated the versions page.

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

Very minor, but for paragraphs in /pages I disabled the text length rule and use soft wrapping in the text editor.

This makes editing long-form documentation easier because we don't need to manage endline placement.

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

Tested locally and looks good!

The final step (if necessary) will be to create any PRs against Studio/Kolibri/KDP with the updates necessary to make sure the copy alias changes don't cause breakage.

thank you!

@indirectlylit indirectlylit merged commit 5a61794 into learningequality:main Jul 12, 2021
@MisRob
Copy link
Member Author

MisRob commented Jul 13, 2021

@indirectlylit I've checked KDP, KDS, and Studio, and no updates are needed there in regards to this PR.

@indirectlylit
Copy link
Contributor

great, thanks for investigating!

@MisRob MisRob deleted the material-icons-upgrade branch October 20, 2021 08:59
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.

2 participants