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

fix: hide the grid-pro editor component until updated #6820

Merged
merged 9 commits into from
Nov 20, 2024
Merged

fix: hide the grid-pro editor component until updated #6820

merged 9 commits into from
Nov 20, 2024

Conversation

tomivirkki
Copy link
Member

@tomivirkki tomivirkki commented Nov 14, 2024 •
edited
Loading

Description

This PR makes GridPro apply the "loading-editor" attribute on the web component while a custom editor component is still being loaded.

Kapture.2024-11-14.at.16.49.01.mp4

Fixes vaadin/web-components#2550

Depends on vaadin/web-components#8156

Type of change

Bugfix

@tomivirkki tomivirkki marked this pull request as draft November 14, 2024 07:51
@tomivirkki tomivirkki force-pushed the fix/grid-pro-custom-editor-old-value-flashing branch from 828c419 to 2ffcf57 Compare November 14, 2024 10:59
@tomivirkki tomivirkki force-pushed the fix/grid-pro-custom-editor-old-value-flashing branch from 2ffcf57 to e7fd908 Compare November 14, 2024 15:00
@tomivirkki tomivirkki mentioned this pull request Nov 14, 2024
Merged
tomivirkki
tomivirkki commented Nov 15, 2024
View reviewed changes
...rent/vaadin-grid-pro-flow/src/main/resources/META-INF/resources/frontend/gridProConnector.js
Comment on lines +18 to +23
if (editor.__itemKey !== itemKey) {
// This is an outdated call that can occur if the user starts editing a cell,
// and quickly starts editing another cell on the same column before the editor
// is unhidden for the first cell.
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't find a way to test this in an IT. Here's the issue it fixes:

Kapture.2024-11-15.at.09.29.28.mp4

@tomivirkki tomivirkki mentioned this pull request Nov 18, 2024
Closed
sissbruecker
sissbruecker reviewed Nov 20, 2024
View reviewed changes
...rent/vaadin-grid-pro-flow/src/main/resources/META-INF/resources/frontend/gridProConnector.js Outdated
Comment on lines 57 to 61
const stopCellEdit = column._stopCellEdit;
column._stopCellEdit = function (editor, itemKey, grid) {
stopCellEdit.call(this, editor, itemKey, grid);
this._grid.toggleAttribute(LOADING_EDITOR_CELL_ATTRIBUTE, false);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

The arguments here are different than what is actually in the web component: https://github.com/vaadin/web-components/blob/bafcfd7d3b9c02af6162444666bbad1f5488b225/packages/grid-pro/src/vaadin-grid-pro-edit-column-mixin.js#L295-L302

Could use apply instead to keep this logic safe for future changes in the WC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@tomivirkki tomivirkki marked this pull request as ready for review November 20, 2024 10:45
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 20, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@tomivirkki tomivirkki merged commit 46679a3 into main Nov 20, 2024
5 checks passed
@tomivirkki tomivirkki deleted the fix/grid-pro-custom-editor-old-value-flashing branch November 20, 2024 11:55
vaadin-bot pushed a commit that referenced this pull request Nov 20, 2024
@tomivirkki @vaadin-bot
fix: hide the grid-pro editor component until updated (#6820)
ec0676e
vaadin-bot pushed a commit that referenced this pull request Nov 20, 2024
@tomivirkki @vaadin-bot
fix: hide the grid-pro editor component until updated (#6820)
7639f38
@vaadin-bot
Copy link
Collaborator

Hi @tomivirkki and @tomivirkki, when i performed cherry-pick to this commit to 24.3, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 46679a3
error: could not apply 46679a3... fix: hide the grid-pro editor component until updated (#6820)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

tomivirkki added a commit that referenced this pull request Nov 20, 2024
@tomivirkki
fix: hide the grid-pro editor component until updated (#6820)
e925ef4
@tomivirkki tomivirkki mentioned this pull request Nov 20, 2024
Closed
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.6.0.beta1 and is also targeting the upcoming stable 24.6.0 version.

sissbruecker added a commit that referenced this pull request Nov 22, 2024
@vaadin-bot @tomivirkki @sissbruecker
fix: hide the grid-pro editor component until updated (#6820) (#6842)
cb206a6 
Co-authored-by: Tomi Virkki <[email protected]>
Co-authored-by: Sascha Ißbrücker <[email protected]>
sissbruecker added a commit that referenced this pull request Nov 26, 2024
@vaadin-bot @tomivirkki @sissbruecker
fix: hide the grid-pro editor component until updated (#6820) (#6843)
8832587 
Co-authored-by: Tomi Virkki <[email protected]>
Co-authored-by: Sascha Ißbrücker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sissbruecker sissbruecker sissbruecker approved these changes

Assignees
No one assigned
Labels
cherry-picked-24.4 cherry-picked-24.5 need to pick manually 24.3 prerelease version for Vaadin 24.6.0 Released with Vaadin 24.6.0.beta1 target/24.3 target/24.4 target/24.5
Projects
None yet
Milestone
No milestone
3 participants
@tomivirkki @vaadin-bot @sissbruecker