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_chatter_position: Migration to 17.0 #2833

Open
wants to merge 16 commits into
base: 17.0
Choose a base branch
from

Conversation

Jp-alitec
Copy link

Migrated to V17.

@Jp-alitec Jp-alitec mentioned this pull request May 23, 2024
26 tasks
@Jp-alitec
Copy link
Author

Jp-alitec commented May 23, 2024

hey @legalsylvain @chienandalu @CRogos i have closed the previous PR (#2826) and created this new one following every guideline , can you please review?

@Rad0van
Copy link

Rad0van commented May 24, 2024

This does not work at all for me. It breaks My Preferences dialog completely and I have a feeling it breaks other dialogs as well. Teste on fresh clean 17.0 It is compileForm(el, params) function breaking it...
image

@Jp-alitec
Copy link
Author

Jp-alitec commented May 24, 2024

@Rad0van Oh ! its because of the optional chaining
(12b405b)
if (child.attributes?.name?.value !== "button_box") {
append(form, this.compileNode(child, params));
}
I have fixed this and code is working as expected but , the optional chaining is not accepted by pre-commit !! as soon as i update this, it fails , what should i do to fix that ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this

Copy link
Author

Choose a reason for hiding this comment

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

@MihranThalhath Sorry but what ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jp-alitec pandoc deb file

Copy link
Author

@Jp-alitec Jp-alitec May 24, 2024

Choose a reason for hiding this comment

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

@MihranThalhath Removed it !

Still
image

Copy link
Member

Choose a reason for hiding this comment

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

And both commits should be squashed together, or there will be 60 MB of diff

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza Sqashed / rebase everything according to guidelines

Copy link
Member

Choose a reason for hiding this comment

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

The binary file is still here.

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza Where , sorry its not visible to me !

Copy link
Member

Choose a reason for hiding this comment

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

imagen

Now to fix it you need to undo the removal in this commit, put it on a separate one, and then squash with the previous commit (pre-commit auto-fixes)

@MihranThalhath
Copy link
Contributor

image
checked on runboat and this is how it looks on invoice view

@Jp-alitec Jp-alitec force-pushed the 17.0-mig-web_chatter_position branch from 8b93b01 to 8412027 Compare May 24, 2024 12:54
@duyquyen96
Copy link

@Jp-alitec PR #2795 (comment) Does this have any effect on PR?

Copy link

@ryantran-novobi ryantran-novobi left a comment

Choose a reason for hiding this comment

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

This pull request works great on my end, even with Web Responsive and Odoo Enterprise.

I suggested a few changes just to have the layout better represented on a big monitor.

Comment on lines +6 to +17
.o_form_view {
.o_form_sheet_bg {
max-width: none;
}
.o_form_sheet {
max-width: $o-form-view-sheet-max-width;
width: 100%;
@include media-breakpoint-up(md) {
margin: $o-sheet-vpadding * 0.2 auto;
}
}
}

Choose a reason for hiding this comment

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

Suggested change
.o_form_view {
.o_form_sheet_bg {
max-width: none;
}
.o_form_sheet {
max-width: $o-form-view-sheet-max-width;
width: 100%;
@include media-breakpoint-up(md) {
margin: $o-sheet-vpadding * 0.2 auto;
}
}
}
.o_form_view {
.o_form_renderer {
max-width: none;
}
.o_form_sheet_bg {
max-width: none;
.o_form_statusbar {
max-width: $o-form-view-sheet-max-width;
width: 100%;
margin: 0px auto;
}
}
.o_form_sheet {
max-width: $o-form-view-sheet-max-width;
width: 100%;
@include media-breakpoint-up(md) {
margin: $o-sheet-vpadding * 0.2 auto;
};
@include media-breakpoint-down(sm) {
width: unset !important;
}
}
}

Choose a reason for hiding this comment

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

☝️ @Jp-alitec

@dustinhoanghavi
Copy link

@Jp-alitec In the Invoice view form, I see duplicate chatter, can you fix it? It could be even better. Thank you!
image

@tva-subteno-it
Copy link

Hi, I tried this module for myself because one my client needs it. It works really well on my end, with no bug. You only have the pre-commit formatting left to do.
What is the stage of progress of this PR?

@icf20
Copy link

icf20 commented Jul 17, 2024

@Jp-alitec I see duplicate chatter, can you fix it?

confirmed

@trisdoan
Copy link
Contributor

@Jp-alitec I see duplicate chatter, can you fix it?

confirmed

Hi @ryantran-novobi, I am working on fixing the duplicate issue and want to include your suggestion. As I tried, I saw no effect on the form view. Could you please elaborate (nicer with images)?

@trisdoan
Copy link
Contributor

@Jp-alitec I see duplicate chatter, can you fix it?

confirmed

Hi @ryantran-novobi, I am working on fixing the duplicate issue and want to include your suggestion. As I tried, I saw no effect on the form view. Could you please elaborate (nicer with images)?

Hello everyone, I fixed the issue and prepared this #2893

Please help to review

@BialArno
Copy link

mhm, oficially OCA module hasn't been migrated, but based on OCA module someone has already published their own version, that seems to work in 17.0 (tho not a single word regarding the usage of OCA module, tho code is 1:1 besides minor changes to form compiler).

Copy link
Contributor

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

be carefull... the *.deb file is still in the commit history and should be removed from the first commit. Everything in the history will blowup the repository size.

image

image

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

Successfully merging this pull request may close these issues.