Skip to content

Conversation

@vursen
Copy link
Contributor

@vursen vursen commented May 19, 2021

Description

Previously, some internal grid components used Polymer templates to render a pre-defined cell content.

The PR introduces the concept of default renders and converts all the internal grid templates to default renderers.

  • ref!: vaadin-grid-selection-column (see breaking change below)
  • ref: vaadin-grid-filter-column
  • ref: vaadin-grid-sort-column
  • ref: vaadin-grid-tree-column

Fixes #1963

Related to #326 (removing the Polymer Template API from grids)

Computed renderers

I found the logic of computing a final renderer to render is quite messy and not obvious to work with. The renderers used to be re-assigned in different observers, so that made it hard to know which renderer is being used at the moment.

After a refactoring, all such the logic has been moved to computed properties, e.g:

Breaking change in the Selection Column

Setting the path property for the selection column is no longer supported.

Before:

const column = document.createElement('vaadin-grid-selection-column');

column.header = 'custom header text'; // Overrides the header cell content

column.path = 'property.0.title'; // Overrides the body cells content

After:

const column = document.createElement('vaadin-grid-selection-column');

column.header = 'custom header text'; // Overrides the header cell content

column.path = 'property.0.title'; // Doesn't affect the column anymore!

Type of change

  • Internal change

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.

@vlukashov vlukashov added the no-polymer Removing Polymer from Vaadin public APIs label May 20, 2021
@vursen vursen force-pushed the ref/vaadin-grid/default-renderers branch 7 times, most recently from 806c896 to adc96bf Compare May 21, 2021 06:23
@vursen vursen force-pushed the ref/vaadin-grid/default-renderers branch from adc96bf to ae672b6 Compare May 24, 2021 10:52
@vursen vursen force-pushed the ref/vaadin-grid/default-renderers branch from ae672b6 to c57394c Compare May 24, 2021 13:05
@vursen vursen changed the title ref: use default renderers instead of internal templates ref!: convert internal templates to default renderers May 24, 2021
@vursen vursen force-pushed the ref/vaadin-grid/default-renderers branch 5 times, most recently from 1f96d3c to 5e5087a Compare May 25, 2021 19:45
@vursen vursen changed the title ref!: convert internal templates to default renderers ref!: convert internal grid templates to renderers May 25, 2021
@vursen vursen marked this pull request as ready for review May 25, 2021 20:18
@vursen vursen requested review from tomivirkki and web-padawan May 25, 2021 20:18
@vursen vursen force-pushed the ref/vaadin-grid/default-renderers branch from 24f1c07 to 507361d Compare May 25, 2021 20:31
@vursen vursen force-pushed the ref/vaadin-grid/default-renderers branch 3 times, most recently from 54e705c to c959eab Compare May 26, 2021 09:14
@vursen vursen force-pushed the ref/vaadin-grid/default-renderers branch from c959eab to f78582b Compare May 26, 2021 09:31
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@web-padawan web-padawan merged commit f90382d into master May 26, 2021
@web-padawan web-padawan deleted the ref/vaadin-grid/default-renderers branch May 26, 2021 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-polymer Removing Polymer from Vaadin public APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

convert internal vadin-grid templates to use renderers instead of <template>s

5 participants