-
Notifications
You must be signed in to change notification settings - Fork 116
[IMP] html_builder: change bring up/down icons #4831
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
base: master-imp-website-image-options-gppa
Are you sure you want to change the base?
[IMP] html_builder: change bring up/down icons #4831
Conversation
|
This PR targets the un-managed branch odoo-dev/odoo:master-imp-website-image-options-gppa, it needs to be retargeted before it can be merged. |
8140797 to
8c29bc2
Compare
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.
The icons look great!
Just a small comment, there are still occurences of .o_send_back and .o_bring_front in odoo/addons/html_builder/static/src/core/overlay_buttons/overlay_buttons.scss if you could check to adapt/remove them, thanks!
8c29bc2 to
1d65cff
Compare
- Persist tooltip 'Double click to replace' for 3 seconds, because users often missed it when it disappeared too quickly. - Update image overlay actions to adopt a toolbar-like style, since the old design looked inconsistent and confused users. task-4789254
- Reorder image options to follow a more logical sequence. - Remove size option for image in grid mode. task-4789254
19ef084 to
a61edf2
Compare
1d65cff to
d76e3b1
Compare
`*=html_builder, html_editor` - Hide overlay buttons when editor toolbar is active as this leads to a confusion for users. task-4789254
a61edf2 to
a65928c
Compare
d76e3b1 to
c898894
Compare
`*=html_builder, html_editor` - Hide overlay buttons when editor toolbar is active as this leads to a confusion for users. task-4789254
This commit adds new icons Odoo UI icons for the send to back/bring to front functionality. task-5173190
c898894 to
fc4f167
Compare
9b53b7e to
74fbb3a
Compare
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.
Here is my review 🙂 The new icons are really nice 🤩
I have some comments, mostly on the naming 👍
|
|
||
| const buttons = []; | ||
| this.overlayTarget = target; | ||
| if (!this.config.isMobileView(this.overlayTarget)) { |
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.
Is it the final "improve toolbar" PR or did you only cherry-pick the commits to see what the new icons will look like on it ? 🙂
If it is not the final PR, can you clean it and keep only what is needed ? 🙂
EDIT: I see that it was already cherry-picked in the other PR, so don't forget to make the changes on it then
For me the new icons could be sent independently but as you prefer 🙂
| .oi-move-up:before { content: '\e84c'; } | ||
| .oi-move-down:before { content: '\e84d'; } |
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.
Not a fan of changing send-back/bring-front for this, you should at least keep the front/back notion => when I read up/down I am expecting arrows or to move vertically, but here they are really used to say in front of/behind so you should keep that 🙂
Could you change it to oi-move-front/back (oi-move-behind could also be a possibility) or oi-bring-front/oi-send-back ? 🙂
|
|
||
| const buttons = []; | ||
| this.overlayTarget = target; | ||
| if (!this.config.isMobileView(this.overlayTarget)) { |
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.
Commit message:
- Based on my other comments, you should therefore rename the commit as
change the grid mode "bring front/send back" icons🙂 new icons Odoo UI icons-> icons two times- also mention the website task it is linked to ->
task-4789254😉
| class: "o_send_back oi", | ||
| class: "oi-move-down oi", | ||
| title: _t("Send to back"), | ||
| handler: this.sendGridItemToBack.bind(this), | ||
| }, | ||
| { | ||
| class: "o_bring_front oi", |
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.
Instead of removing the old classes, I would have only removed the CSS associated to them and added your new classes, as they were used to target these specific grid buttons easily in JS too (and they matched the button handlers) 🤔 (its like the clone button: it uses fa fa-clone but still has oe_snippet_clone to identify it 👍 )
If you rename them as asked in my other comment, I guess it could be fine, but I still think they should be easily "identifiable" so let's keep the old classes 🙂
This PR adds new icons Odoo UI icons for the send to back/bring to front functionality.
task-5173190