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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form buttons should always get a (correct) btn style class #24443

Closed
wants to merge 2 commits into from

Conversation

qsm-odoo
Copy link
Contributor

@qsm-odoo qsm-odoo commented Apr 26, 2018

Related to task-49276

Here is the story of the problem: back in 10.0, everything seemed
fine with button rendering except it was very messy duplicated codes.
The system was still not consistent:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-default btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-default btn-link
oe_link -> btn-default oe_link
btn-danger -> btn-default btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> no class
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> oe_link
btn-danger -> btn-danger
some_class -> some_class

So, outside the header, the system seemed to not process anything except
transforming the oe_highlight class. In the header, the btn-default was
always added, except if using the oe_highlight class. The style is fine
for those, except for .btn-default.btn-link and .btn-default.oe_link
where the link class has no effect (which is fine as header are not
supposed to contain links).

In 11.0, the new views were introduced alongside websitepocalypse. The
new system led to:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-default btn-primary
oe_highlight -> btn-default oe_highlight
btn-default -> btn-default
btn-link -> btn-default btn-link
oe_link -> btn-default oe_link
btn-danger -> btn-default btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> oe_highlight
btn-default -> btn-default
btn-link -> btn-link
oe_link -> oe_link
btn-danger -> btn-danger
some_class -> some_class

So, it worsened the header button by not processing the oe_highlight
class and adding the btn-default class in all cases. This is even
worse since in 11.0 .btn-default.btn-link do have a specific style and
thus not appear as .btn-default anymore. Those may be invisible "bugs"
though as .btn-link are not supposed to be in header and oe_highlight
is styled correctly in css. For buttons outside the header, a little
processing was added to add "btn-default" class for buttons which did
not specified a class (and oe_highlight is not processed anymore there
too).

In master, with the less2scss task, the system had to change again but
led this time to visible bugs:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

As you can see, the system is now the same for both header button and
normal buttons. The system is also better for all cases as it produce
only one bootstrap class for all cases. However, the problematic
case is the last one for header buttons: it did not receive the
btn-default class automatically. Even though this is the API for the
rest of odoo (dialogs, renderButton (old WidgetButton), ...), this is
a problem here as fixing this would require to add the 'btn-default'
class in every view which is using this case.

=> This commit is fixing that, see the commit message 馃槃

@qsm-odoo qsm-odoo added the Framework General frontend/backend framework issues label Apr 26, 2018
@qsm-odoo qsm-odoo self-assigned this Apr 26, 2018
@qsm-odoo
Copy link
Contributor Author

@ged-odoo I introduced the type option as discussed yesterday but this in fact worsen the problem as it solves the bug for header buttons but creates one for sheet buttons while adding crappy btn-default classes like before. Should I make the saas team hate me even more by going through all views to add the btn-default class where needed ?

@aab-odoo Any opinion on this ?

@aab-odoo
Copy link
Contributor

@qsm-odoo The previous solution (where only one bootstrap class was present) was cleaner, imho.

Do we really want the 'btn-default' className to be added on every button rendered by the framework? I'm not sure that this is a good idea.

Can't we handle the case of header buttons a little bit differently, by forcing (only in this case) at least a btn-default className if there is no other bootstrap class? So basically, render the button normally (using the utils), then check if the resulting button has a 'btn-something' className, and if not, add 'btn-default'? We want to have the full control on the rendering of the header, so I don't see any problem with forcing at least 'btn-default' className, but only in that case.

cc @ged-odoo

@qsm-odoo
Copy link
Contributor Author

@aab-odoo
Yes, it is of course cleaner without crappy btn-default classes.
Yes, we can do that (and I even did at some point), the line is just also ugly but I also think it is the best solution. I will adapt the code (I still think the type option is meaningful though so I will keep it).

@C3POdoo C3POdoo added the RD research & development, internal work label Apr 26, 2018
In particular, allow to create a button without a sizing class.
Here is the story of the problem: back in 10.0, everything *seemed*
fine with button rendering except it was very messy duplicated codes.
The system was still not consistent:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-default btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-default btn-link
oe_link -> btn-default oe_link
btn-danger -> btn-default btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> no class
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> oe_link
btn-danger -> btn-danger
some_class -> some_class

So, outside the header, the system seemed to not process anything except
transforming the oe_highlight class. In the header, the btn-default was
always added, except if using the oe_highlight class. The style is fine
for those, except for .btn-default.btn-link and .btn-default.oe_link
where the link class has no effect (which is fine as header are not
supposed to contain links).

In 11.0, the new views were introduced alongside websitepocalypse. The
new system led to:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-default btn-primary
oe_highlight -> btn-default oe_highlight
btn-default -> btn-default
btn-link -> btn-default btn-link
oe_link -> btn-default oe_link
btn-danger -> btn-default btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> oe_highlight
btn-default -> btn-default
btn-link -> btn-link
oe_link -> oe_link
btn-danger -> btn-danger
some_class -> some_class

So, it worsened the header button by not processing the oe_highlight
class and adding the btn-default class in all cases. This is even
worse since in 11.0 .btn-default.btn-link do have a specific style and
thus not appear as .btn-default anymore. Those may be invisible "bugs"
though as .btn-link are not supposed to be in header and oe_highlight
is styled correctly in css. For buttons outside the header, a little
processing was added to add "btn-default" class for buttons which did
not specified a class (and oe_highlight is not processed anymore there
too).

In master, with the less2scss task, the system had to change again but
led this time to visible bugs:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

As you can see, the system is now the same for both header button and
normal buttons. The system is also better for all cases as it produce
only *one* *bootstrap* class for all cases. However, the problematic
case is the last one for header buttons: it did not receive the
btn-default class automatically. Even though this is the API for the
rest of odoo (dialogs, renderButton (old WidgetButton), ...), this is
a problem here as fixing this would require to add the 'btn-default'
class in every view which is using this case.

While waiting for a better solution, this commit is instead forcing
the btn-default class in that header case only (so only when a class
was mentioned but it did not contain any bootstrap btn type class).
So the new system gives:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

This commit also adds tests which will prevent the system to change
again without noticing it.
@qsm-odoo
Copy link
Contributor Author

@aab-odoo I removed the type option, it was annoying me. I added more tests and split the commit in two.

Copy link
Contributor

@aab-odoo aab-odoo left a comment

Choose a reason for hiding this comment

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

Seems good

qsm-odoo added a commit that referenced this pull request Apr 26, 2018
Here is the story of the problem: back in 10.0, everything *seemed*
fine with button rendering except it was very messy duplicated codes.
The system was still not consistent:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-default btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-default btn-link
oe_link -> btn-default oe_link
btn-danger -> btn-default btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> no class
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> oe_link
btn-danger -> btn-danger
some_class -> some_class

So, outside the header, the system seemed to not process anything except
transforming the oe_highlight class. In the header, the btn-default was
always added, except if using the oe_highlight class. The style is fine
for those, except for .btn-default.btn-link and .btn-default.oe_link
where the link class has no effect (which is fine as header are not
supposed to contain links).

In 11.0, the new views were introduced alongside websitepocalypse. The
new system led to:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-default btn-primary
oe_highlight -> btn-default oe_highlight
btn-default -> btn-default
btn-link -> btn-default btn-link
oe_link -> btn-default oe_link
btn-danger -> btn-default btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> oe_highlight
btn-default -> btn-default
btn-link -> btn-link
oe_link -> oe_link
btn-danger -> btn-danger
some_class -> some_class

So, it worsened the header buttons by not processing the oe_highlight
class and adding the btn-default class in all cases. This is even
worse since in 11.0 .btn-default.btn-link do have a specific style and
thus not appear as .btn-default anymore. Those may be invisible "bugs"
though as .btn-link are not supposed to be in header and oe_highlight
is styled correctly in css. For buttons outside the header, a little
processing was added to add "btn-default" class for buttons which did
not specify a class (and oe_highlight is not processed anymore there
too).

In master, with the less2scss task, the system had to change again but
led this time to visible bugs:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

As you can see, the system is now the same for both header button and
normal buttons. The system is also better for all cases as it produce
only *one* *bootstrap* class for all cases. However, the problematic
case is the last one for header buttons: it did not receive the
btn-default class automatically. Even though this is the API for the
rest of odoo (dialogs, renderButton (old WidgetButton), ...), this is
a problem here as fixing this would require to add the 'btn-default'
class in every view which is using this case.

While waiting for a better solution, this commit is instead forcing
the btn-default class in that header case only (so only when a class
was mentioned but it did not contain any bootstrap btn type class).
So the new system gives:

-> In Header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> btn-default some_class

-> Not in header, button with:
no class -> btn-default
btn-primary -> btn-primary
oe_highlight -> btn-primary
btn-default -> btn-default
btn-link -> btn-link
oe_link -> btn-link
btn-danger -> btn-danger
some_class -> some_class

This commit also adds tests which will prevent the system to change
again without noticing it.

Closes #24443
@qsm-odoo
Copy link
Contributor Author

Still don't know why PR are sometimes closed by the 'Closes ...' mention and sometimes not. Anyways, this was closed with b2ad88e

@qsm-odoo qsm-odoo closed this Apr 30, 2018
@aab-odoo
Copy link
Contributor

aab-odoo commented May 2, 2018

@qsm-odoo It only works for stuff merged in 11.0 (the master branch), i think

@xmo-odoo xmo-odoo deleted the master-fix-btn-default-qsm branch November 20, 2019 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework General frontend/backend framework issues RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants