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

No individual shadow roots for each separate cell, better content selectors #488

Merged
merged 1 commit into from
Oct 14, 2016

Conversation

tomivirkki
Copy link
Member

@tomivirkki tomivirkki commented Oct 13, 2016

These fixes mainly target grid with native shadow DOM on:

  • Each cell doesn't need to have individual shadow roots (just the content selector)
  • Class names aren't the right choice for content selectors in this case, using attributes
  • Light dom items shouldn't accumulate on dom changes (for example when number of columns change)
  • vaadin-grid-table-cell implementation is now more in line with other vaadin-grid-table- components

This change is Reviewable

@Saulis
Copy link
Contributor

Saulis commented Oct 14, 2016

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


vaadin-grid-table-cell.html, line 126 at r1 (raw file):

          // not helping here for some reason.
          this.async(function() {
            if (this.target.domHost) {

Comment: looks like this logic doesn't work perfectly when grid is nested inside a dom-bind template, domHost is then null. However, vaadin-grid seems to be assigned with a x-scope class. I'll file a separate issue about that.


Comments from Reviewable

@Saulis Saulis merged commit a263ed4 into 2.0-dev Oct 14, 2016
@Saulis Saulis deleted the fix/2.0-cell-content branch October 14, 2016 10:34
@Saulis Saulis removed the in review label Oct 14, 2016
HJK181 pushed a commit to CommerceExperts/vaadin-grid that referenced this pull request Jul 27, 2020
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.

None yet

2 participants