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

[ADD] extensions: icon_role font awesome #8469

Closed
wants to merge 1 commit into from

Conversation

samueljlieber
Copy link
Contributor

@samueljlieber samueljlieber commented Apr 1, 2024

Task: #3843168

This PR adds a custom extension for an :icon: role to allow the use of Font Awesome icons in documentation.

With the new role, icons can be called by their classname.

This is a Font Awesome :icon:`fa-step-forward` icon.
Screenshot 2024-04-02 at 11 07 01 AM

If this PR is approved, it will likely need to be manually forward ported to each version.

@samueljlieber samueljlieber self-assigned this Apr 1, 2024
@robodoo
Copy link
Collaborator

robodoo commented Apr 1, 2024

@C3POdoo C3POdoo requested review from a team April 1, 2024 13:22
@samueljlieber
Copy link
Contributor Author

Hello @odoo/doc-review and @odoo/design-doc-review 👋 I have an exciting PR that implements icons into documentation via a new custom role. I decided to make this custom role its own extension, rather than add it directly to the odoo_theme, however I added the icon sets the role uses to the odoo_theme.

I tested icon RST formatting in all of the directives, and the :card: directive is the only one that the new :icon: role does not format in (title or body). I did not have any issues building with latex either, however, I did not try to use the :icon: role in the build.

The need to implement icons into Odoo documentation stems from writing effectively for the end-user. By using the UI icons from Odoo in the documentation, we can directly reference the visual elements of the product when instructing the user. This has many benefits:

  • Increases the skim-ability of the document
  • Provides clarity of information
  • Reduces technical writer load

The US Content Marketing team has had to resort to using emojis and icon descriptors to instruct the user for UI elements. This has resulted in a cumbersome review process and multiple variations of emojis and descriptions used to depict an icon.

Im looking forward to hearing your thoughts on this implementation of icons in documentation! :)

Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

Hello, good initiative! :)

Given that this role is specific to Odoo and its resources will live in the odoo_theme directory, a separate extension is not needed.

Regarding the rendering of the role, I think we should limit it to just the <icon>, instead of <icon> (<icon_class>). The reasons are that the writer might want to hide or rename the class, and I'm not convinced that the class would be translatable anyway.

conf.py Outdated Show resolved Hide resolved
@samueljlieber
Copy link
Contributor Author

Hi @AntoineVDV, thank you for your insightful review :) in de2dd98 I moved the role code to odoo_theme and removed any icon_role extension declarations I had in the conf.py. I also removed the :guilabel: icon descriptor feature from the icon_role() function, so now only the icon is rendered. Should be ready for another look 👍

@AntoineVDV
Copy link
Collaborator

I'll give a few days to the design team to have a look before I make a final pass ;)

@Brieuc-brd
Copy link

Hello 👋
Can you tell me where I can see an example of icon rendering in your branch?
Is there a specific page in the documentation?

@C3POdoo C3POdoo requested a review from a team April 8, 2024 12:48
@samueljlieber
Copy link
Contributor Author

Hello 👋 Can you tell me where I can see an example of icon rendering in your branch? Is there a specific page in the documentation?

Hi @Brieuc-brd, in 20b0760 I added a section in content/contributing/documentation/rst_cheat_sheet.rst for icons so you can see the formatting– we will likely remove this content add before merge to keep this PR's changes strictly to the implementation of the :icon: role, but for now feel free to use this list of all icon classes in RST that I generated for testing.

See the icon section here: https://runbot145.odoo.com/runbot/static/build/60967664-15-0/logs/build/html/contributing/documentation/rst_cheat_sheet.html#icons

@jcs-odoo
Copy link
Contributor

jcs-odoo commented Apr 8, 2024

THIS IS SUPER EXCITING!

Copy link

@Brieuc-brd Brieuc-brd left a comment

Choose a reason for hiding this comment

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

Hello @samueljlieber 👋
Thanks for your work 👍

I've done some tests on runbot and I'm concerned about a few things:

  • This PR targets documentation for Odoo 15, while oi icons were introduced in 16.0. This means that if the documentation uses these icons, users using Odoo before 16.0 won't find them in the backend.
    IMO, oi should be added from 16.0.
    Here's an example below :
In the Doc In the backend
Capture d’écran 2024-04-08 à 16 34 20 Capture d’écran 2024-04-08 à 16 32 14
  • oi icons have been created to fit the backend's font base, which is 14px. In the Documentation, the font base is 16px, which creates antialiasing when you render oi icons. I think a CSS adjustment would be necessary :)
Current Expected
Capture d’écran 2024-04-08 à 16 39 50 Capture d’écran 2024-04-08 à 16 41 20
  • What about classes to handle sizes and fixed widths ? (fa-lg =>fa-5x, oi-small => oi-larger, fa-fw & oi-fw)
    Is this something you don't need to add in the Documentation?

  • Also, I don't see adaptations for RTL (https://github.com/odoo/odoo/blob/master/addons/web/static/lib/odoo_ui_icons/style.css#L63), is it something you don't need in the Documentation ?

  • Odoo UI icons is a library that will evolve in the future (icons may be added or updated in future versions). Do you have any plans to keep the library up to date for Documentation? :)

I think adding FontAwesome and UI icons in the Documentation is a good idea, but we need to be sure that everything is well defined to maintain consistency with the Backend :)

Courage 💪

cc. @stefanorigano

extensions/odoo_theme/static/scss/_odoo_ui_icon.scss Outdated Show resolved Hide resolved
@samueljlieber
Copy link
Contributor Author

Thank you so much @Brieuc-brd for your thorough review and feedback on this PR- it is greatly appreciated 🙏 I want to share my thoughts on some of your points while I am working on implementing corrections for others.

This PR targets documentation for Odoo 15, while oi icons were introduced in 16.0. This means that if the documentation uses these icons, users using Odoo before 16.0 won't find them in the backend.
IMO, oi should be added from 16.0.

  • We certainly should only include the version specific icons. I agree oi should be added from 16.0, this PR can still target 15.0, however, I will be removing oi from its contents and adding that in, in the 16.0 version of the PR.

oi icons have been created to fit the backend's font base, which is 14px. In the Documentation, the font base is 16px, which creates antialiasing when you render oi icons. I think a CSS adjustment would be necessary :)

  • This is a great catch. I believe a simple add of the font-size: 14px; rule in extensions/odoo_theme/static/scss/_font_awesome.scss would do the trick.

What about classes to handle sizes and fixed widths ? (fa-lg =>fa-5x, oi-small => oi-larger, fa-fw & oi-fw)
Is this something you don't need to add in the Documentation?

  • Personally, I think that this would add more complications than what is needed for documentation. At the moment I don't see an instance where we would need to "alter" icons in this way in docs, however, I am completely open to everyones thoughts on the matter. @jcs-odoo and @StraubCreative do you see any need to have different sizes and fixed widths options when using icons in docs? (also see the next point on rotation) ⬇️

Also, I don't see adaptations for RTL, is it something you don't need in the Documentation ?

  • I also don't think we need adaptations for RTL as these adaptations seem to be already covered by existing icons. For example, oi-arrow-down-right with the o_rtl adaptation just becomes oi-arrow-up-left correct?

Odoo UI icons is a library that will evolve in the future (icons may be added or updated in future versions). Do you have any plans to keep the library up to date for Documentation? :)

  • Most definitely, I believe the implementation of this :icon: role lends itself to being flexible with the icon classes used. If/when Odoo UI is expanded, an improvement PR can be made on the documentation repo to include those classes. A good workflow would likely be periodically reviewing the library and making adjustments on doc quarterly.

Looking forward to continuing the discussion here, so the implementation of icons in docs is accurate and consistent with Odoo and set up to be scalable as we progress! Thanks again!

@samueljlieber
Copy link
Contributor Author

Hi everyone, in f1e54b6 I did the following:

  • Removed oi icons, leaving this PR to only introduce Font Awesome to 15.0. oi will be introduced from 16.0+.
  • Added the 14px font base to extensions/odoo_theme/static/scss/_font_awesome.scss to correct the antialiasing.
  • Modified to support handling sizes, fixed widths, and colors (e.g. fa-code-fork fa-flip-vertical text-success)

Please see the RST Cheatsheet Icons section for formatting examples.

I am curious on your thoughts on including the color variations (text-success, text-warning, etc) in documentation. I think it is a good idea to exactly match the UI, and it is helpful when documenting the icon color changes that occur on the UI after an event.

Thank you!

@samueljlieber samueljlieber changed the title [ADD] extensions: icon_role odoo ui and font awesome [ADD] extensions: icon_role font awesome Apr 22, 2024
@xpl-odoo
Copy link
Contributor

Thanks for the initiative, @samueljlieber ! It should really help both documentation writers and Odoo users alike.

@AntoineVDV
Copy link
Collaborator

@Brieuc-brd Did you want to make another pass?

Copy link

@Brieuc-brd Brieuc-brd left a comment

Choose a reason for hiding this comment

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

Hey ! @samueljlieber 👋
Thanks for your work 👍

I've left a few comments below 😉

extensions/odoo_theme/static/scss/_font_awesome.scss Outdated Show resolved Hide resolved
extensions/odoo_theme/static/scss/_font_awesome.scss Outdated Show resolved Hide resolved
extensions/odoo_theme/static/scss/_font_awesome.scss Outdated Show resolved Hide resolved
extensions/odoo_theme/static/scss/_font_awesome.scss Outdated Show resolved Hide resolved
@Brieuc-brd
Copy link

@samueljlieber
I forgot something: :icon: should return a <i/> tag instead of <span/>.

@samueljlieber
Copy link
Contributor Author

I forgot something: :icon: should return a <i/> tag instead of <span/>.

@Brieuc-brd is using the <i> element a requirement of the icon libraries or purely semantic? I am asking because I believe this is a limitation of Docutils, the <inline> element used to generate the :icon: role renders as a <span>, and I am unaware of a way to specify using the <i> element 🤔 https://docutils.sourceforge.io/docs/ref/doctree.html#inline

There is an inline <emphasis> element that returns an <em> tag rather than an <i>.

@samueljlieber
Copy link
Contributor Author

samueljlieber commented Apr 24, 2024

590ddca Implemented @Brieuc-brd 's suggestions, except for the <i> element– see my reasoning.

I also implemented @jcs-odoo 's suggestion.

@Brieuc-brd
Copy link

is using the <i> element a requirement of the icon libraries or purely semantic?

Both 😅

the element used to generate the :icon: role renders as a <span>, and I am unaware of a way to specify using the <i> element 🤔 https://docutils.sourceforge.io/docs/ref/doctree.html#inline

I'm not an expert on the subject, perhaps @AntoineVDV would have an idea on how to proceed? 🙂

Copy link

@Brieuc-brd Brieuc-brd left a comment

Choose a reason for hiding this comment

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

LGTM, good job 👍

Copy link
Collaborator

@AntoineVDV AntoineVDV left a comment

Choose a reason for hiding this comment

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

Thanks @Brieuc-brd for the design review 👍

@samueljlieber Good work!
I pushed some changes in a fixup! commit to:

  • use register_canonical_role which should be more appropriate;
  • remove the second call to handle_error because empty roles seems not to be interpreted;
  • merge handle_error into icon_role since it's small enough and called in only one place;
  • use the provided text variable rather than building full_text which, from what I can see, is essentially test + a space character at the end that is not interpreted when used in a class attribute;
  • insert back a inliner.problematic node into the rendered document.
    If that's ok for you, let's merge 🚀

@robodoo delegate+

@AntoineVDV AntoineVDV removed the request for review from a team April 26, 2024 08:01
Copy link
Contributor

@jcs-odoo jcs-odoo left a comment

Choose a reason for hiding this comment

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

Hi everyone :)

Here are some cases (in 15.0) where a class could be useful to be defined, as the color of the icon indicates a status:

  • activities (in Project, for example)
    • o_activity_color_planned
    • o_activity_color_planned
    • o_activity_color_overdue
  • messages status in the chatter (if the mail bounces back for example)
  • the components' availability in manufacturing orders
    • No class on the FA icon, but the parent container has a green btn-linkclass
    • text-danger
  • the favorite button, although it's not a class, but a different FA icon
    • one is specific to Odoo
    • the other is the FA icon but with a custom yellow color I think

However, the Odoo-related classes are not included here, and I wouldn't use the text-info, text-success, text-danger, and text-warning classes just to display a resembling color, since the only occurrence I could find was the use of text-danger in Manufacturing.

Unless we add "Odoo classes" to this repo, and define clearly when to use them or not in the writing guidelines, I think it's best to avoid mentioning that we can add classes in the RST cheat sheet.

In any case, I think we'll have to add some instructions in the writing guidelines when we revisit them :)

What do you think?

@samueljlieber
Copy link
Contributor Author

Thank you @AntoineVDV for the fixup– I agree with your changes 👍

However, the Odoo-related classes are not included here, and I wouldn't use the text-info, text-success, text-danger, and text-warning classes just to display a resembling color, since the only occurrence I could find was the use of text-danger in Manufacturing.

Unless we add "Odoo classes" to this repo, and define clearly when to use them or not in the writing guidelines, I think it's best to avoid mentioning that we can add classes in the RST cheat sheet.

@jcs-odoo I agree with you that we should use the exact Odoo color classes and not substitute with the FA classes. I think you make some really good points.

My main concern with using colored icons is standardizing how they are used– I worry that having Odoo color classes and FA color classes with FA icons (and eventually Odoo UI icons in 16.0+) is a lot of complexity to standardize in the doc guidelines, and may cause a lot of overhead for writers & reviewers.

Im starting to think it may be best to just use black icons, and describe any color changes– also we still have the fill vs outline variations of icons. Ultimately, adding icons (with no color) is a huge step in precisely documenting the UI– is showing the color of the icon that necessary when we can just state it in the instruction?

Click on the :icon:`fa-clock-o` :guilabel:`(clock)` icon to schedule an activity, and proceed to fill out the pop-up form.

Activity colors, and their relation to an activity’s due date, is consistent throughout Odoo, regardless of the activity type, or the view.

  • Green indicate a due date sometime in the future.
  • Yellow indicates that the activity’s due date is today.
  • Red indicates that the activity is overdue and the due date has passed.

As exciting as adding colors to icons is, I fear it adds more complexity than value. WDYT?

If we agree to not use colors, I will remove the SUPPORTED_ICON_CLASSES check in the icon_role 🙂

@jcs-odoo
Copy link
Contributor

Indeed, let's not use color classes for now.
Thanks for you work :)

@samueljlieber
Copy link
Contributor Author

samueljlieber commented Apr 29, 2024

3ebf78f removed SUPPORTED_ICON_CLASSES from the icon_role() and removed the FA color class examples from rst_cheet_sheet.rst, squashed and rebased.

@samueljlieber
Copy link
Contributor Author

9428a84 Updated commit message:
[ADD] extensions: icon_role odoo ui and font awesome ➡️ [ADD] extensions: icon_role font awesome

@samueljlieber
Copy link
Contributor Author

Merging and FWP up to master. I will be opening a new PR to introduce Odoo UI icons in 16.0+.
Thanks everyone! 🚀
@robodoo r+

robodoo pushed a commit that referenced this pull request Apr 29, 2024
closes #8469

Signed-off-by: Samuel Lieber (sali) <sali@odoo.com>
@robodoo robodoo closed this Apr 29, 2024
@jcs-odoo
Copy link
Contributor

congrats ^^

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

6 participants