Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add editCell to gridPro #3354

Closed
wants to merge 13 commits into from
Closed

Add editCell to gridPro #3354

wants to merge 13 commits into from

Conversation

roastedcpu
Copy link
Contributor

@roastedcpu roastedcpu commented Jan 26, 2022

Description

Adds trigger from the backend side to open the cell editor.

Fixes # (issue)

Type of change

[ ] Bugfix

  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

throw new Error('Invalid colIdx (out of bounds)');
}

const column = columns[colIdx];
Copy link
Member

Choose a reason for hiding this comment

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

Column reordering isn't taken into account

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, you can pass a columnId and that ensures reordering is not a problem. What do you suggest for situations where the index is directly passed ?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC the API design accepts "path" only


// Make sure row[rowIdx] exists
if (rowIdx > tRows.length || rowIdx < 0) {
throw new Error('Invalid rowIdx (out of bounds)');
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't sound right to throw an error depending on the scroll position of the user & how many items the virtual scrolling engine has decided to render in DOM?

@@ -455,6 +455,63 @@ export const InlineEditingMixin = (superClass) =>
super._updateItem(row, item);
}

/**
* @param {Number | String} row
Copy link
Member

Choose a reason for hiding this comment

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

Examining the code, seems that row could also be an item?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not anymore, but I'd like this possibility, especially because of scrolling. Passing the index only works well if all items are being shown.

<vaadin-grid-pro-edit-column path="name" id="name"></vaadin-grid-pro-edit-column>
<vaadin-grid-pro-column path="age"></vaadin-grid-pro-column>
<vaadin-grid-column>
<template>[[item.name]]</template>
Copy link
Member

Choose a reason for hiding this comment

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

Don't use the deprecated template API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just copied from other examples. How should I do it ?

Copy link
Member

Choose a reason for hiding this comment

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

In case the column isn't essential for the test, it can be removed. If it is, see the documentation for how custom content is provided for grid column cells (renderer API)

throw new Error('Invalid rowIdx (out of bounds)');
}

this._startEdit(tRows[rowIdx].cells[colIdx], column);
Copy link
Member

Choose a reason for hiding this comment

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

The rowIdx probably refers to a virtual index and not the index of the element in the DOM (which wouldn't be correct for the API due to virtual scrolling). If rowIdx was, let's say 1000, this would fail since there are not that many physical rows in the DOM.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@yuriy-fix
Copy link
Contributor

Closing as stale. Not all of the comments were addressed by the contributor and the approach is not consistent with the existing APIs.

Feel free to reopen if still relevant.

@yuriy-fix yuriy-fix closed this May 31, 2022
@sirbris
Copy link

sirbris commented May 31, 2022

Hi, so no triggering editor mode via code? Is there any workaround?

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

Successfully merging this pull request may close these issues.

[gridpro] Add editCell(..) method to programmatically activating cell editor
4 participants