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.

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

[16.0][ADD] pos_screen_element_custom_size #1158

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from
Open

[16.0][ADD] pos_screen_element_custom_size #1158

wants to merge 1 commit into from

Conversation

geomer198
Copy link
Contributor

This module allows to define custom size values for certain POS screen elements.

@geomer198 geomer198 force-pushed the 16.0-t3340-pos_screen_element_custom_size-add branch from 74db397 to 9994dd7 Compare February 20, 2024 21:00
legalsylvain
legalsylvain requested changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Interesting module. thanks for sharing !
could you remove the pandoc... .deb file ?

thanks !

@geomer198 geomer198 force-pushed the 16.0-t3340-pos_screen_element_custom_size-add branch from 9794027 to 35f202b Compare February 21, 2024 10:14
@legalsylvain legalsylvain added this to the 16.0 milestone Feb 21, 2024
@geomer198 geomer198 force-pushed the 16.0-t3340-pos_screen_element_custom_size-add branch from 35f202b to 4fa73ce Compare February 21, 2024 15:43
@geomer198
Copy link
Contributor Author

Interesting module. thanks for sharing ! could you remove the pandoc... .deb file ?

thanks !

Could you check again?

@cvinh
Copy link
Contributor

cvinh commented Feb 21, 2024

Interesting module. thanks for sharing ! could you remove the pandoc... .deb file ?
thanks !

Could you check again?

Do you have any idea how the pandoc deb appeared ? I had the same in one of my PR once...

@geomer198
Copy link
Contributor Author

Interesting module. thanks for sharing ! could you remove the pandoc... .deb file ?
thanks !

Could you check again?

Do you have any idea how the pandoc deb appeared ? I had the same in one of my PR once...

Pandoc is added after running pre-commit.

ivs-cetmix
ivs-cetmix approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@ivs-cetmix ivs-cetmix 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 LGTM
@legalsylvain would appreciate your code review when you have time)

legalsylvain
legalsylvain approved these changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@legalsylvain legalsylvain 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 no test. LGTM.

Note : could be great to have also a min height. when product has no images, the button can be too little.

image

not a blocking point, can be added later.

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

@dreispt
Copy link
Sponsor Member

dreispt commented Mar 22, 2024

/ocabot merge nobump

1 similar comment
@legalsylvain
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1158-by-legalsylvain-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 28, 2024
@OCA-git-bot
Merge PR #1158 into 16.0
8d2a137 
Signed-off-by legalsylvain
@OCA-git-bot
Copy link
Contributor

@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1158-by-legalsylvain-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@legalsylvain
Copy link
Contributor

Hi @geomer198 could you rebase and take a look on red ci ? Thanks !

@geomer198 geomer198 force-pushed the 16.0-t3340-pos_screen_element_custom_size-add branch from 4fa73ce to 460207f Compare May 1, 2024 20:26
@ivs-cetmix
Copy link
Member

hi @legalsylvain could you please check again? Thank you in advance!

@legalsylvain
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-1158-by-legalsylvain-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request May 10, 2024
@OCA-git-bot
Merge PR #1158 into 16.0
eb431e3 
Signed-off-by legalsylvain
@OCA-git-bot
Copy link
Contributor

@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1158-by-legalsylvain-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

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

@ivs-cetmix ivs-cetmix ivs-cetmix approved these changes

@legalsylvain legalsylvain legalsylvain approved these changes

Assignees
No one assigned
Labels
approved ready to merge
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@geomer198 @cvinh @OCA-git-bot @dreispt @legalsylvain @ivs-cetmix