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

[MIG] web_responsive: Migrate to v12 and refactor #1066

Merged
merged 54 commits into from Jan 10, 2019

Conversation

yajo
Copy link
Member

@yajo yajo commented Oct 5, 2018

This migration includes a full refactoring to make this module more
maintainable. Some things that have changed:

  • Removed external libraries.
  • Change Less for Scss.
  • Reduce ES and XML to the minimal required needs.
  • Implement as much features as possible with just Scss.
  • Use the new hotkeys system from Odoo v12.

@Tecnativa

@yajo yajo added this to the 12.0 milestone Oct 5, 2018
@yajo yajo self-assigned this Oct 5, 2018
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 5, 2018
40 tasks
Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

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

Small Change

web_responsive/README.rst Outdated Show resolved Hide resolved
web_responsive/README.rst Outdated Show resolved Hide resolved
web_responsive/README.rst Outdated Show resolved Hide resolved
web_responsive/README.rst Outdated Show resolved Hide resolved
web_responsive/__init__.py Outdated Show resolved Hide resolved
@yajo
Copy link
Member Author

yajo commented Oct 8, 2018

@nikul-serpentcs all done 😉

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Very cool!

I found a small issue in Chrome:

  • When you open the apps box it's not possible to navigate with the keys. No response from the arrows and the tab kicks you out of the menu.

In the aesthetics side, wouldn't it be nice to have a colored background in the apps box?

@yajo
Copy link
Member Author

yajo commented Oct 9, 2018

When you open the apps box it's not possible to navigate with the keys. No response from the arrows and the tab kicks you out of the menu.

App navigation is not supported right now. Only navigating through results when you search. I'll add it to roadmap.

In the aesthetics side, wouldn't it be nice to have a colored background in the apps box?

I suppose, but I wanted to focus on getting it functional. It can be beautier later 😉. Adding to roadmap as well.

@yajo
Copy link
Member Author

yajo commented Oct 9, 2018

Roadmap updated.

@chienandalu
Copy link
Member

@yajo I mean the search results. It's not possible to select them with the keys in Chrome. It works perfectly in Firefox though.

@yajo
Copy link
Member Author

yajo commented Oct 9, 2018

Oh sorry! Let me check that.

@yajo
Copy link
Member Author

yajo commented Oct 9, 2018

It should work now. Problem: https://stackoverflow.com/a/11031754/1468388

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Great work! 👍 👍 👍

Copy link
Contributor

@aitorbouzas aitorbouzas left a comment

Choose a reason for hiding this comment

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

Love to see this module migrated! Nice work, thanks for your effort.
Just a small note.

web_responsive/models/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

I love the search function because it searches on display_name.
I've also did a functional test. It seems some features are lost because there's no keyboard navigation to change the app selection anymore. I don't know if other features are lost too.

web_responsive/static/src/js/web_responsive.js Outdated Show resolved Hide resolved
web_responsive/static/src/css/web_responsive.scss Outdated Show resolved Hide resolved
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Functional review. Great improvement on the search!

Several points to discuss/solve:

  • Arrows don't work on app switcher for selecting directly the app and press enter.
    There are 2 solutions for this:

    • Present the app icon also on the list of candidates, and remove all of them when you have typed anything
    • when you press the arrows on the text box, you navigate on the apps icons.
  • Apps icons are moved down and top each time you type something on the search box. The first solution for the previous point will solve this also.

  • On Firefox, switching to mobile view, it's not possible to search, as I can't type in the textbox. It's curious, as I have been able to do it on a real mobile, so this is not critical.

  • As the search happens in another "screen", can you hide the searchbox on mobile view and let only the glass icon? This way, we gain a bit of space.

  • We have lost the improvements made on v11 for collapsing statusbar and buttons. Please restore it:

    seleccion_005

    About the name of the button, I'm still not very convinced for calling it "Tasks". Enterprise responsive interface calls this "Action", but hides "Print" and "Action" dropdown that are when full. I consider this a loss, as you can't for example remove a record from mobiles. Is it feasible to join the 3 of them (Print dropdown, Action dropdown and buttons) in the same mobile dropdown, split by a separator? Enterprise also increases the space for tapping each option and put text in upper for better recognition.

  • On a notebook with pages that doesn't fit current width, full page appears side scrollable, which breaks mobile purpose. Enterprise makes scrollable in this case only the pages part instead of everything. Other technique can be to collapse extra pages that doesn't fit on mobile width (or put all of them as collapsable?)

    seleccion_006

    EDIT: Looking to other form views, it's an incorrect size of the notebook container, as here we don't have too many pages and they fit on the width:

    seleccion_008

  • On mobile view, shadow and padding is preserved on forms with sheet attribute. We should gain this space:

    seleccion_007

@Tardo can you review this also?

Copy link
Member

@Tardo Tardo left a comment

Choose a reason for hiding this comment

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

As said by others... good work with search feature!

Bad news:

  • Can't use shortcuts, browser interpret it as "accelerator key" (ALT+)
  • Settings pages doesn't display properly
  • Sticky feature has been lost
  • You can move out of the "search list bounds" when use arrow keys
  • Search-box dispatch events when press modifier keys
  • I see some app names "cutted":
    app_title

@pedrobaeza
Copy link
Member

Another pending thing is the inclusion of web_searchbar_full_width module in this one, which is a simply CSS rule.

@osoul

This comment has been minimized.

@pedrobaeza

This comment has been minimized.

@osoul

This comment has been minimized.

@pedrobaeza

This comment has been minimized.

@rafaelbn
Copy link
Member

rafaelbn commented Nov 1, 2018

Maybe we can discard this module as has a high maintenance an start using

It is already done and supported by Openworx

@pedrobaeza
Copy link
Member

AFAIK that one is not responsive. And IMO is ugly. It doesn't include menu search nor app switcher (only a variant of the navbar). And finally, it's not OCA.

@mgielissen
Copy link

mgielissen commented Nov 2, 2018

AFAIK that one is not responsive. And IMO is ugly. It doesn't include menu search nor app switcher (only a variant of the navbar). And finally, it's not OCA.

The first version only has the app sidebar and more enterprise look'n'feel. The second version will using web_responsive when it's available.

@pedrobaeza
Copy link
Member

@yajo what is the status of this?

@yajo
Copy link
Member Author

yajo commented Dec 12, 2018

Under a pile of work. 🙄

@Yakulu Yakulu mentioned this pull request Dec 14, 2018
@pedrobaeza
Copy link
Member

You might need to modify something when odoo/odoo#29994 is merged.

@yajo yajo force-pushed the 12.0-web_responsive branch 3 times, most recently from b8fb90c to 7c4da85 Compare January 10, 2019 11:22
@yajo
Copy link
Member Author

yajo commented Jan 10, 2019

I fixed all concerns. Some new screenshots/gifs:

  • View type switcher on mobiles:
    peek 10-01-2019 10-58

  • The control panel is hidden when scrolling, to save screen space, but the main top bar is always visible:
    peek 10-01-2019 11-02

  • Form status bar action buttons are collapsed in a dropdown.
    Other control panel buttons use icons to save space.
    peek 10-01-2019 11-21

  • Breadcrumbs navigation is collapsed with a "back arrow" button.
    peek 10-01-2019 11-15

All these gifs appear in the updated README now.

This is ready to review.

This migration includes a full refactoring to make this module more
maintainable. Some things that have changed:

- Removed external libraries.
- Change Less for Scss.
- Reduce ES and XML to the minimal required needs.
- Implement as much features as possible with just Scss.
- Remove copyright from `__init__.py` files.
- Trigger the new hotkeys system from Odoo v12 with `Shift+Alt` instead
  of just `Alt`, and restore some good old hotkeys (`E` for "Edit",
  `D` for "Discard", and `A` for "Apps menu").
  See odoo/odoo#30068 on the matter.
- Control panel breadcrumbs are collapsed into a single backwards icon.
- Add FA icons to most common buttons in control panel.
- Hide text in XS for those buttons, to have a slicker phone experience.
- Lots of gifs in the README!
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

All the comments have been attended, so I'm going to merge this after a long incubation phase.

@yajo full width searchbar is still missing, but we can add it later on a new PR.

@pedrobaeza pedrobaeza merged commit 4d10311 into OCA:12.0 Jan 10, 2019
@pedrobaeza pedrobaeza deleted the 12.0-web_responsive branch January 10, 2019 15:32
@steync027
Copy link

Great work!

@mgielissen
Copy link

When will it be published in the Odoo Apstore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet