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

[IMP] theme_beauty, *: rename the font "Muli" to "Mulish" #634

Closed
wants to merge 1 commit into from

Conversation

rdeodoo
Copy link
Contributor

@rdeodoo rdeodoo commented Mar 7, 2023

  • theme_bookstore, theme_clean, theme_enark, theme_kiddo, theme_loftspace, theme_odoo_experts, theme_orchid

This commit will rename the font "Muli" to "Mulish" as it has been renamed in Google Fonts.

Closes #574

@rdeodoo rdeodoo force-pushed the master-fix-muli-rename-rde branch from bd76447 to 4418c82 Compare March 7, 2023 18:10
@rdeodoo rdeodoo requested a review from qsm-odoo March 7, 2023 18:10
@robodoo
Copy link
Collaborator

robodoo commented Mar 7, 2023

@rdeodoo
Copy link
Contributor Author

rdeodoo commented Mar 7, 2023

Despite not being found in google (both by URL tweak or by search), it is still served for now.

Let's clean that now so we don't forget about it, in case they later remove the Muli compatibility.
Probably could be done in earlier version but better than nothing for now.
In 3 years we probably won't even think about it anymore and all stable version will have it 👍

@rdeodoo rdeodoo force-pushed the master-fix-muli-rename-rde branch from 4418c82 to aac3dc2 Compare March 7, 2023 18:13
Copy link
Contributor

@qsm-odoo qsm-odoo left a comment

Choose a reason for hiding this comment

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

What about doing what JKE said here: https://github.com/odoo/design-themes-old/pull/432#issuecomment-755237524 (using $o-fonts-similar)? (alongside what you did here of course). Without that, a customer migrating to 17.0 will have a broken font 😉 (until we can properly migrate them).

And if you do that, I guess it would be nice to just target 16.0 with this. Would allow to have one extra year of safety in case Google drops the support for Muli 🤷 (and I am sure the 16.0 and master version is the same, not worth it checking before).

@rdeodoo
Copy link
Contributor Author

rdeodoo commented Mar 8, 2023

a customer migrating to 17.0 will have a broken font 😉

Why? It is just a text entry which at the end calls a google font URL with that name?
They would still be serving Muli as I mentioned from Google which still serves it (for now, probably not forever?).
I might be missing something?

That's why I created this task so we don't wait too long and hopefully when Google will stop serving this font for compatibility, all our stable will already use Mulish.

Only problem remaining will be the DB using Muli if Google ever drop the support.

We could take more time to write an upgrade script and a compatibility stuff in scss indeed, but then this will postpone this fix again (we could have merged the initial one > 2 years ago) as I don't have time for it now since it's very low priority (just took 3 minutes to revive this one and not waste a few more years).

@qsm-odoo
Copy link
Contributor

qsm-odoo commented Mar 8, 2023

@rdeodoo

Why? It is just a text entry which at the end calls a google font URL with that name?

No, it won't 😬 By doing this change:
image

You change the name of the font entry in that map. A user who chose it have this in its user_values.scss file:

'font': 'Muli',

Since you rename the 'Muli' key to something else, the migrated user won't have a font anymore because there won't be any 'Muli' entry in the maps you modified.

Arguably, you could only change the "family" and "url" values and leave the key untouched, but that would lead to displaying the wrong name in the editor.

We could take more time to write an upgrade script [...]

No, as always: we don't have access to migrate this as this is in attachments.

[...] and a compatibility stuff in scss indeed but then this will postpone this fix

That's what I am suggesting, there is already something for that: just one entry added in the $o-fonts-similar map will solve all issues here 😉 You can r+ without it if you prefer not spending the extra time... but just know that 16.2/3 users will lose their font immediately (not when Google drops his support).

* theme_bookstore, theme_clean, theme_enark, theme_kiddo,
  theme_loftspace, theme_odoo_experts, theme_orchid

This commit will rename the font "Muli" to "Mulish" as it has been
renamed in Google Fonts.

Closes odoo#574

Courtesy of Kaushalya Mandaliya <kma@odoo.com>
@rdeodoo rdeodoo force-pushed the master-fix-muli-rename-rde branch from aac3dc2 to 0caebce Compare March 9, 2023 13:38
@rdeodoo rdeodoo changed the base branch from master to 16.0 March 9, 2023 13:44
@rdeodoo
Copy link
Contributor Author

rdeodoo commented Mar 9, 2023

there won't be any 'Muli' entry in the maps you modified.

Indeed, that's what I was missing 👍

Created the community PR too

Copy link
Contributor

@qsm-odoo qsm-odoo left a comment

Choose a reason for hiding this comment

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

@robodoo
Copy link
Collaborator

robodoo commented Mar 9, 2023

@rdeodoo @qsm-odoo linked pull request(s) odoo/odoo#114810 not ready. Linked PRs are not staged until all of them are ready.

robodoo pushed a commit to odoo/odoo that referenced this pull request Mar 10, 2023
This commit will rename the font "Muli" to "Mulish" as it has been
renamed in Google Fonts.

closes #114810

Related: odoo/design-themes#634
Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
@robodoo robodoo closed this in 7afe3a3 Mar 10, 2023
@robodoo robodoo temporarily deployed to merge March 10, 2023 11:25 Inactive
@fw-bot fw-bot deleted the master-fix-muli-rename-rde branch March 24, 2023 11:46
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.

Rename Muli to Mulish?
3 participants