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_select_all_companies: Migration to 17.0 #2834

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

Conversation

Jp-alitec
Copy link

@Jp-alitec Jp-alitec commented May 24, 2024

Migrated to V17

@Jp-alitec Jp-alitec mentioned this pull request May 24, 2024
26 tasks
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.

One thing I am not sure. I think, when you click the very first time, it is not working. If I see it correctly, you can also recognize this in the description gif recording.

Could you cleanup the git history according to the migration guide?
image

@CRogos
Copy link
Contributor

CRogos commented May 24, 2024

here is also another PR: #2798

@Jp-alitec Jp-alitec force-pushed the 17.0-mig-web_select_all_companies branch from 5c64c7c to 7b82b9e Compare May 24, 2024 11:01
@Jp-alitec
Copy link
Author

@CRogos Merge/squash done .

Also in this PR i have tested "when you click the very first time, it is not working" this issue is not there.
Also it has some additional style changes that differ from V16 (the font size , hover styles ).

also i think in the existing PR #2798 branches might not be get selected from this functionality whereas in this PR that issue is also fixed.

image

@Jp-alitec Jp-alitec requested a review from CRogos May 24, 2024 11:36
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.

Could you fix the commit message and the version number.
Rest LGTM.

Could not reproduce the first click issue eigther.

image

# License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html).

{
"name": "Web Select All Companies",
"summary": "Allows you to select all companies in one click.",
"version": "16.0.1.0.1",
"version": "17.0.1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

version should be 17.0.1.0.0

Copy link
Author

Choose a reason for hiding this comment

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

@CRogos Version updated and fixed the commit.

@Jp-alitec Jp-alitec force-pushed the 17.0-mig-web_select_all_companies branch 2 times, most recently from 1dfdc69 to fc1d06d Compare May 24, 2024 12:16
@Jp-alitec Jp-alitec requested a review from CRogos May 24, 2024 12:17
@Jp-alitec Jp-alitec force-pushed the 17.0-mig-web_select_all_companies branch from fc1d06d to aa229e2 Compare May 24, 2024 12:19
@Jp-alitec
Copy link
Author

@legalsylvain Please review.

@yannoliv
Copy link

yannoliv commented Jun 3, 2024

Hey! I am the guy from the other PR #2798. I have tested both our implementations on runboat and they are practically the same. Our solution looks very similar.

@Jp-alitec
Copy link
Author

Hey! I am the guy from the other PR #2798. I have tested both our implementations on runboat and they are practically the same. Our solution looks very similar.

@yannoliv try to create branches and then select all companies, there the difference will be visible.

@Jp-alitec
Copy link
Author

@legalsylvain @CRogos What is the next step to get this PR merged ?

@StefanRijnhart
Copy link
Member

@Jp-alitec I'll test it if you rebase to get runboat to build.

<div
role="button"
tabindex="0"
class="d-flex flex-grow-1 align-items-center py-0 log_into ps-2"

Choose a reason for hiding this comment

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

Suggested change
class="d-flex flex-grow-1 align-items-center py-0 log_into ps-2"
class="d-flex flex-grow-1 align-items-center py-0 ps-2"

IMHO log_intohas to be removed here, since it is (was?) linked to JS event to "log into a company" and it is irrelevant for "All companies".

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.

10 participants