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

ref!: convert internal grid templates to renderers #1967

Merged
merged 10 commits into from
May 26, 2021

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
4 participants