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] web: widget ribbon scss revamp #99336

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

morm-odoo
Copy link
Contributor

Prior to this PR, there was unnecessary scss for the ribbon widget.
A lot of template are not converted to owl yet, so we added the ribbon scss to the legacy ui.scss file, and it has to be removed after the migration to owl is complete.

task-2968399

@robodoo
Copy link
Contributor

robodoo commented Aug 31, 2022

Pull request status dashboard

@morm-odoo morm-odoo force-pushed the master-web-widget-ribbon-scss-revamp-morm branch from 33a0790 to c3087da Compare August 31, 2022 13:32
@C3POdoo C3POdoo added the RD research & development, internal work label Aug 31, 2022
addons/web/static/src/legacy/scss/ui.scss Outdated Show resolved Hide resolved
addons/web/static/src/views/widgets/ribbon/ribbon.scss Outdated Show resolved Hide resolved
text-transform: uppercase;
text-align: center;
overflow: hidden;
user-select: none;
&.o_small {
font-size: 12px;
Copy link
Contributor

Choose a reason for hiding this comment

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

small class or <small> tag ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those classes are added via JS
Screenshot 2022-09-01 at 16 07 49

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Is it a problem?

z-index: 1;
position: absolute;
display: grid;
align-items: center;
width: 225px;
height: 48px;
Copy link
Contributor

Choose a reason for hiding this comment

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

These width/height values look arbitrary :/
Can we use a different approach?

@@ -1,8 +1,6 @@
.ribbon {
width: 150px;
height: 150px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Arbitrary values... why exactly 150px?
Can't we, for example, let the browser compute these dimensions based on the content?
If it's not possible can we use variables instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't change those value dynamically without affecting the border of the ribbon, so I added a scss variable file

addons/web/static/src/views/widgets/ribbon/ribbon.xml Outdated Show resolved Hide resolved
@morm-odoo morm-odoo force-pushed the master-web-widget-ribbon-scss-revamp-morm branch from c3087da to 021e759 Compare September 1, 2022 14:07
Copy link
Contributor

@stefanorigano stefanorigano left a comment

Choose a reason for hiding this comment

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

Thanks @morm-odoo , some remarks ;)

@@ -38,11 +23,10 @@

&-top-right {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class seems redundant since it's added to the main container by default.
Could you traceback the history of the class and check if it's really needed?

@@ -38,11 +23,10 @@

&-top-right {
margin-top: -$o-sheet-vpadding;
Copy link
Contributor

Choose a reason for hiding this comment

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

The component is supposed to be rendered also outside formView's...
does this value works for other views too? I would be surprised.
Investigate this, eventually I propose to use css variables, something like:

Suggested change
margin-top: -$o-sheet-vpadding;
margin-top: var(--ribbon-margin-top, #{-$o-sheet-vpadding});
. o_kanban_record & {
--ribbon-margin-top: #{$o-kanban-inside-vgutter};
}

@@ -13,21 +11,8 @@
}
Copy link
Contributor

Choose a reason for hiding this comment

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

border: 5px solid #2980b9;
why 🥺

@@ -63,12 +47,3 @@
}
Copy link
Contributor

Choose a reason for hiding this comment

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

& span {
    left: -15px;
    top: 30px;
    [...]
}

These left and top values... 30px, -15px... can we replace them with something less arbitrary ?

@morm-odoo morm-odoo force-pushed the master-web-widget-ribbon-scss-revamp-morm branch 2 times, most recently from b60d111 to 18bd271 Compare September 2, 2022 13:50
@morm-odoo
Copy link
Contributor Author

@stefanorigano can you check if you agree with my new version of the ribbon? :)

Prior to this commit, there was unnecessary scss for the ribbon widget.
A lot of template are not converted to owl yet, so we added the scss to
the legacy ui.scss file, and it has to be removed after the migration to
 owl.

task-2968399
Prior to this commit, some scss related to the status (state selection
component) was not at the right place
Prior to this commit, the scss related to the status (state selection
component) was not optimal.
This commit add a scss variable file for the ribbon widget.
Prior to this commit, there was unnecessary scss for the ribbon widget.
The same design is possible to get with a simpler css.
@morm-odoo morm-odoo force-pushed the master-web-widget-ribbon-scss-revamp-morm branch from 18bd271 to 791cb82 Compare January 5, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants