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

[17.0][MIG] web_refresher: Migration to 17.0 #2669

Merged
merged 36 commits into from
Nov 24, 2023

Conversation

TesaLazarenko
Copy link
Contributor

No description provided.

josueBulle and others added 30 commits November 15, 2023 12:27
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: web-16.0/web-16.0-web_refresher
Translate-URL: https://translation.odoo-community.org/projects/web-16-0/web-16-0-web_refresher/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: web-16.0/web-16.0-web_refresher
Translate-URL: https://translation.odoo-community.org/projects/web-16-0/web-16-0-web_refresher/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: web-16.0/web-16.0-web_refresher
Translate-URL: https://translation.odoo-community.org/projects/web-16-0/web-16-0-web_refresher/
Currently translated at 100.0% (2 of 2 strings)

Translation: web-16.0/web-16.0-web_refresher
Translate-URL: https://translation.odoo-community.org/projects/web-16-0/web-16-0-web_refresher/hr/
Currently translated at 50.0% (1 of 2 strings)

Translation: web-16.0/web-16.0-web_refresher
Translate-URL: https://translation.odoo-community.org/projects/web-16-0/web-16-0-web_refresher/fr/
@legalsylvain
Copy link
Contributor

/ocabot migration web_refresher

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Nov 15, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Nov 15, 2023
26 tasks
@pedrobaeza
Copy link
Member

Tested on runbot and working.

question: Is there a reason to put the button on the center instead of next to the pager as always?

@TesaLazarenko
Copy link
Contributor Author

Tested on runbot and working.

question: Is there a reason to put the button on the center instead of next to the pager as always?

image

Hi, there were a few reasons for that, in previous versions, page refresh was not available on all views where it was most often needed, in the current version I attach this in not only pager, which allows you to get this functionality almost everywhere. In the image of odoo 15, as you can see, the update functionality is not available, so placing it in the search component solves this problem

@TesaLazarenko TesaLazarenko reopened this Nov 16, 2023
@pedrobaeza
Copy link
Member

But we worked on that on all the versions:

Original in 13.0: #2243
Forward-port for 15.0: #2365
Forward-port for 16.0: #2366

so if it's not working completely in 16.0, it should be fixed, but putting in the middle seems a bit weird.

@TesaLazarenko
Copy link
Contributor Author

But we worked on that on all the versions:

Original in 13.0: #2243 Forward-port for 15.0: #2365 Forward-port for 16.0: #2366

so if it's not working completely in 16.0, it should be fixed, but putting in the middle seems a bit weird.

If someone else can confirm that this position is strange or inconvenient, I'll move this to the pager as it was before, but I'm not sure that this functionality will be available on reports

@TesaLazarenko
Copy link
Contributor Author

But we worked on that on all the versions:
Original in 13.0: #2243 Forward-port for 15.0: #2365 Forward-port for 16.0: #2366
so if it's not working completely in 16.0, it should be fixed, but putting in the middle seems a bit weird.

If someone else can confirm that this position is strange or inconvenient, I'll move this to the pager as it was before, but I'm not sure that this functionality will be available on reports

now I'm working on the preliminary look, it looks like everything could be fine

@TesaLazarenko
Copy link
Contributor Author

image


async onClickRefresh() {
await this.refresh();
this.notification.add(_t("Refreshed"), {
Copy link
Member

Choose a reason for hiding this comment

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

No very convinced with this :) Is it really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No very convinced with this :) Is it really needed?

Are you talking about this.notification.add or the entire component?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, just the notification of the refresh :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not necessary, but I noticed in my experience that if you do not display anything, it is not immediately clear whether the click has worked, in a situation where there were no new changes to update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think it's unnecessary or distracting, I can remove it

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a shorter notification close timeout would be less anoying

Copy link
Contributor Author

@TesaLazarenko TesaLazarenko Nov 17, 2023

Choose a reason for hiding this comment

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

ok, I'll think about a simplified version of the notification, as a last resort I'll just remove it.
I'll deal with it later.

Any other comments on other points?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ezgif com-video-to-gif
What about this animation instead of notification (the red ring is added by the screen recorder)

Maybe a shorter notification close timeout would be less anoying

Copy link

@jcdrubay jcdrubay left a comment

Choose a reason for hiding this comment

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

Test on runboat, it works very nicely. Nice :)

@pedrobaeza pedrobaeza changed the title [MIG] web_refresher: Migration to 17.0 [17.0][MIG] web_refresher: Migration to 17.0 Nov 24, 2023
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.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-2669-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot OCA-git-bot merged commit 84ab4d7 into OCA:17.0 Nov 24, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 0958651. Thanks a lot for contributing to OCA. ❤️

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.