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

[14.0] [mig] pos_hide_banknote_button #631

Merged
merged 7 commits into from
Jul 8, 2021

Conversation

dsolanki-initos
Copy link
Contributor

No description provided.

@chienandalu chienandalu added this to the 14.0 milestone Apr 27, 2021
@dsolanki-initos
Copy link
Contributor Author

@chienandalu can you please review this PR?

.pos .payment-numpad .numpad button:first-child,
.pos .payment-numpad .numpad button:nth-child(2),
.pos .payment-numpad .numpad button:nth-child(3),
.pos .payment-numpad .numpad button:last-child {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this new way is less predictive that the previous one. If other OCA modules are adding / altering numpad buttons, it could generate conflicts.

don't you think ?

Not sure how fix it thought.

Otherwise, lgtm, tested on runbot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but there is no other class which I can use to apply the css. If you have the otherway let me know about it.

@chienandalu chienandalu mentioned this pull request Apr 28, 2021
16 tasks
Copy link
Contributor

@fshah-initos fshah-initos left a comment

Choose a reason for hiding this comment

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

LGTM. Functional Test and Code review

@brendapaniagua
Copy link

LGTM!! Functional review

@dsolanki-initos
Copy link
Contributor Author

It would be greate if we could merge this PR.

@dsolanki-initos
Copy link
Contributor Author

Hi @chienandalu can you please merge this PR?

@dsolanki-initos
Copy link
Contributor Author

Hi @chienandalu it would be great if you could merge this PR.

@legalsylvain
Copy link
Contributor

Hi @dsolanki-initos, fshah-initos

First, thanks for porting many OCA / pos (and other repos) modules to the 14.0.
Some rules regarding reviews and merge :

  • To be accepted, a PR should have two approvals by independent reviewers (preferably from another company) during 5 days. See OCA convention : https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/oca_module_lifecycle_development_status.rst
  • Repo maintainers are receiving notification from github where new PR are created, or updated. No needs to ping them each week !
  • For the OCA to work, people need to read twice as many PRs as they offer, preferably from people from other companies, to discover other manner to work and develop. So, while waiting for your PRs to be validated, you can take the opportunity to review others.

I'll take a look in a few weeks on your PRs, but for the time being, i have no time for the review. I'm currently working on 12.0 revision. Recent version (V14) introduced a lot of changes in the framework and the structure (introduction of owl, etc...) I need to understand these changes a little more to make valid code reviews.

Regards.

@dsolanki-initos
Copy link
Contributor Author

dsolanki-initos commented Jun 11, 2021

Hi @legalsylvain,

I hope you are doing good!

I've counted two reviews for this PR and then requested the maintainer for Merge. For this PR we got two reviews one from Foram-Initos and second brendapaniagua I don't know we counted the same or not.

We understand that repo maintainers are busy with other works. Repo maintainers are receving notfications but we are not getting any response so we tag them as we need to merge our PR soon.

We do reviews for other's PR. We have not measured for have many PR's we have done review. If you want we can do so.
Do let us know the PR's where the reviews are needed so that we can add reviews over there.

Thank you!

Regards,
Dhara-Initos

@dsolanki-initos
Copy link
Contributor Author

It would be nice if we get the reviews here.

@legalsylvain
Copy link
Contributor

@dsolanki-initos.

I've counted two reviews for this PR and then requested the maintainer for review. For this PR we got two reviews one from Foram-Initos and second brendapaniagua I don't know we counted the same or not.

@brendapaniagua : please use the approve button of github to make your approval visible, and catchable by oca bot.

Well, @dsolanki-initos, it was a general remarks, regarding all the new PR that initos is doing on OCA/pos. For exemple :

People who are interested in pos (maintainers or not) subscribe to the repo, and thus receive a notification (or mail) when a new PR is created. (but they also receive one for each new post!). So, thanks for reducing the noise on this repo !

we are not getting any response so we tag them as we need to merge our PR soon.

In the meantime, having unmerged PR is a normal situation, and you can continue to work with the branch, without beeing blocked.

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-631-by-legalsylvain-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c1e905b into OCA:14.0 Jul 8, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 12326ba. 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.

8 participants