Skip to content

[18.0][IMP] web_form_banner: adj to resolve eslint warnings #3391

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.

Sign up for GitHub

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

Jump to bottom
Open
AungKoKoLin1997 wants to merge 1 commit into
base: 18.0
Choose a base branch
from
Open

[18.0][IMP] web_form_banner: adj to resolve eslint warnings #3391

AungKoKoLin1997 wants to merge 1 commit into from
+27 −13

Conversation

@AungKoKoLin1997
Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 commented Dec 15, 2025 •
edited
Loading

This is the warnings before this PR.
image

This is after the PR, all warning will be gone.
@qrtl QT6169

yostashiro
yostashiro approved these changes Dec 17, 2025
View reviewed changes
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

LGTM.

nobuQuartile
nobuQuartile approved these changes Dec 17, 2025
View reviewed changes
Copy link

@nobuQuartile nobuQuartile left a comment

Choose a reason for hiding this comment

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

LGTM

m-deura
m-deura approved these changes Dec 17, 2025
View reviewed changes
Copy link

@m-deura m-deura left a comment

Choose a reason for hiding this comment

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

LGTM!

@AungKoKoLin1997
Copy link
Contributor Author

AungKoKoLin1997 commented Dec 17, 2025

@oca/web-maintainers
Could we get this minor fix PR merged?

@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Dec 20, 2025

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). 🤖

1 similar comment
@OCA-git-bot
Copy link
Contributor

OCA-git-bot commented Dec 20, 2025

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). 🤖

etobella
etobella reviewed Dec 24, 2025
View reviewed changes
etobella
etobella reviewed Dec 24, 2025
View reviewed changes
web_form_banner/static/src/js/web_form_banner.esm.js Outdated
return fb;
};

/* eslint-disable no-inline-comments */
Copy link
Member

@etobella etobella Dec 24, 2025

Choose a reason for hiding this comment

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

Why don't you move the comments to the line before. This way you are not disabling a lint condition

Copy link
Contributor Author

@AungKoKoLin1997 AungKoKoLin1997 Dec 24, 2025

Choose a reason for hiding this comment

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

I want to reduce the line of codes as much as possible and the comments are very short enough to use as a inline comment in my opinion.

Copy link
Member

@etobella etobella Dec 24, 2025

Choose a reason for hiding this comment

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

I know that the diff will be longer, however, in a migration it is expected to have this kind of changes. I would prefer to avoid this kind of disables

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 18.0-fix-linter-web_form_banner branch from e858161 to 1e9d18b Compare December 24, 2025 10:07
yostashiro
yostashiro approved these changes Dec 25, 2025
View reviewed changes
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

Code review. Thanks @etobella for your advice!

etobella
etobella approved these changes Dec 27, 2025
View reviewed changes
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Technical review

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

Reviewers

@etobella etobella etobella approved these changes

@nobuQuartile nobuQuartile Awaiting requested review from nobuQuartile

+2 more reviewers

@yostashiro yostashiro yostashiro approved these changes

@m-deura m-deura m-deura approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AungKoKoLin1997 @OCA-git-bot @yostashiro @etobella @m-deura @nobuQuartile