-
Notifications
You must be signed in to change notification settings - Fork 10.2k
[IMP] Attendances: updating images and formatting #12980
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @larm-odoo looks good to me, just a couple of things to note.
There is another image on this doc that has not been updated but still has the red markups on it (we are trying to move away from marked up images)
Also you will want to assign this doc to yourself!
8e40f57
to
e14f95a
Compare
Hi @jero-odoo - thanks for reminding me to assign myself- I hadn't done that for ANY of my PR's =D. |
Hi @samueljlieber - this is ready for a tech review. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @larm-odoo ! Remember to add the points to the Label section of the PR!
.. image:: check_in_check_out/top-menu.png | ||
:align: center | ||
:alt: Top right main menu with check in button highlighted. | ||
of what application the user is in, a :guilabel:`🔴 (red circle)` or :guilabel:`🟢 (green circle)` is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we phased out the emojis, but looking at the HTML for the icon, the circle icon has no color ):
Are there workarounds for this @samueljlieber, or do you think we should just fly with the emojis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Felicious @larm-odoo
I recommend we describe the color like so, formatting the :icon: as usual:
of what application the user is in, a :guilabel:`🔴 (red circle)` or :guilabel:`🟢 (green circle)` is | |
of what application the user is in, a :icon:`fa-circle` :guilabel:`(red circle)` or | |
:icon:`fa-circle` :guilabel:`(green circle)` is |
LMK your thoughts!
Coloring icons is possible, and we discussed it when first implementing icons here: #8469 (review)
We decided not to use colors in icons or text in favor of writing out the UI color changes.
For your reference, this is one way we could color icons in documentation. Notice in your screenshot the text-success
class that follows the icon's class. This is one of the Bootstrap color classes. The following RST will not work without first modifying the :icon: role's definition.
of what application the user is in, a :icon:`fa-circle text-danger` :guilabel:`(red circle)` or
:icon:`fa-circle text-success` :guilabel:`(green circle)` is

A larger discussion is needed around the standards/guidelines for icon colors and how we want to implement them in documentation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are both good and valid. I personally prefer the color only because the UI is colored, and seeing a black circle then describing it as 'red' or 'green' is not my favorite- but it IS consistent with how we do other icons. When I wrote this doc, I know this was discussed, and we had originally landed on the colored circle icons that were in here, and I left them in and did not update them since I thought it was an exception, since we hashed it out when this was created.
Since we have the colored dots available, I do like the second option @samueljlieber presented, but I will absolutely just use the black dot and describe it, if that's what we want to do, @Felicious. Not sure who to listen to here, so I will await a reply to this comment =D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I JUST re-read this, and see that the colored dots are not visible unless we make some modifications. Since that's never a good ide-a I put in the black dots and the text descriptor. Sorry about the confusion!
e14f95a
to
62cfa78
Compare
Hi @samueljlieber - I believe this is now yours! I re-read your comment regarding the dots (I did leave those in on purpose, because I thought it was an exception since it was hashed out a while ago). I changed them so they are both just black dots with descriptions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @larm-odoo, thank you for your work on this PR! Approving with a few comments, please update all emojis to icons in this doc before merging.
Thank you!
..
@robodoo delegate=larm-odoo
62cfa78
to
c6ff2d5
Compare
@robodoo r+ |
Updating to new formatting standards. Images have annotations that need to be removed, one image is extra/unnecessary.