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

Rename chart 'dataLabels' to 'labels'; throw deprecation warning if the former is used. #1989

Merged
merged 24 commits into from
Jan 25, 2022

Conversation

MadsBuchmann
Copy link
Contributor

@MadsBuchmann MadsBuchmann commented Jan 10, 2022

Which issue does this PR close?

This PR closes #1905

What is the new behavior?

The input property dataLabels has been renamed to labels:

  • References in the documentation has been updated to reflect the renaming
  • References in the code has been updated to reflect the renaming
  • dataLabels still works but will throw a deprecation warning

Besides that, i also ended up removing some unused imports along with a bit of rewording and -formating of the documentation.

Does this PR introduce a breaking change?

  • Yes
  • No

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be automatically merged to master via automerge.

@MadsBuchmann MadsBuchmann changed the title Rename chart 'dataLabels' to 'labels' & throw deprecation warning if 'dataLabels' is used. Rename chart 'dataLabels' to 'labels'; throw deprecation warning if the former is used. Jan 10, 2022
@MadsBuchmann MadsBuchmann enabled auto-merge (squash) January 11, 2022 12:27
@MadsBuchmann MadsBuchmann requested a review from mictro January 13, 2022 07:42
? isStockType
? this.getDefaultStockLabels(datasets, this.locale)
: this.createBlankLabels(datasets)
: dataLabels;
: labels;

Copy link
Contributor

@mictro mictro Jan 17, 2022

Choose a reason for hiding this comment

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

A bit hard to read double ternary: Consider something along the lines:

const labelsToApply = labels;
if (!labelsToApply) {
// No labels supplied - autogenerate default labels for the chart.
labelsToApply = isStockType ? this.getDefaultStockLabels(datasets, this.locale) : this.createBlankLabels(datasets);
}

Copy link
Contributor Author

@MadsBuchmann MadsBuchmann Jan 18, 2022

Choose a reason for hiding this comment

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

Modified the suggestion to rely on a const instead of let as i prefer to use immutable variables when i can get away with it. It has been changed to:

    const labelsToApply = (() => {
      if (labels?.length > 0) return labels;
      else if (type === 'stock') return this.getDefaultStockLabels(datasets, this.locale);
      else return this.createBlankLabels(datasets);

    })();

mictro
mictro previously approved these changes Jan 17, 2022
Copy link
Contributor

@mictro mictro left a comment

Choose a reason for hiding this comment

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

LGTM
Jeg har lagt en lille kommentar du kan overveje....

@github-actions github-actions bot temporarily deployed to pr-1905-rename-chart-data-labels January 18, 2022 10:00 Inactive
@MadsBuchmann MadsBuchmann disabled auto-merge January 18, 2022 11:55
@mictro
Copy link
Contributor

mictro commented Jan 18, 2022

ezgif-5-6998636485

@mictro mictro closed this Jan 18, 2022
@mictro mictro reopened this Jan 18, 2022
@github-actions github-actions bot temporarily deployed to pr-1905-rename-chart-data-labels January 18, 2022 12:54 Inactive
@MadsBuchmann MadsBuchmann merged commit a0da285 into master Jan 25, 2022
@MadsBuchmann MadsBuchmann deleted the enhancement/1905-rename-chart-data-labels branch January 25, 2022 12:35
MadsBuchmann added a commit that referenced this pull request Jan 28, 2022
updateDataLabels was renamed to updateLabels in PR #1989.
@mictro mictro linked an issue Feb 2, 2022 that may be closed by this pull request
MadsBuchmann added a commit that referenced this pull request Feb 3, 2022
* Only redraw chart if its rendered

* ♻️ Spy on 'updateLabels' instead of 'updateDataLabels'

updateDataLabels was renamed to updateLabels in PR #1989.

* Updated chart property name & tests

Co-authored-by: Tobias Secher <bdytos@bankdata.dk>
Co-authored-by: Mads Buchmann Frederiksen <mbf@prochimp.dk>
RasmusKjeldgaard added a commit that referenced this pull request Feb 14, 2022
* Only redraw chart if its rendered (#2017)

* Only redraw chart if its rendered

* ♻️ Spy on 'updateLabels' instead of 'updateDataLabels'

updateDataLabels was renamed to updateLabels in PR #1989.

* Updated chart property name & tests

Co-authored-by: Tobias Secher <bdytos@bankdata.dk>
Co-authored-by: Mads Buchmann Frederiksen <mbf@prochimp.dk>

* ⬆️ Bump follow-redirects from 1.13.3 to 1.14.7 (#1994)

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.13.3 to 1.14.7.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.13.3...v1.14.7)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: RasmusKjeldgaard <rkk@bankdata.dk>

* ⬆️ Bump log4js from 6.3.0 to 6.4.0 (#2004)

Bumps [log4js](https://github.com/log4js-node/log4js-node) from 6.3.0 to 6.4.0.
- [Release notes](https://github.com/log4js-node/log4js-node/releases)
- [Changelog](https://github.com/log4js-node/log4js-node/blob/master/CHANGELOG.md)
- [Commits](log4js-node/log4js-node@v6.3.0...v6.4.0)

---
updated-dependencies:
- dependency-name: log4js
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: RasmusKjeldgaard <rkk@bankdata.dk>

* ⬆️ Bump nanoid from 3.1.22 to 3.2.0 (#2005)

Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.22 to 3.2.0.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md)
- [Commits](ai/nanoid@3.1.22...3.2.0)

---
updated-dependencies:
- dependency-name: nanoid
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: RasmusKjeldgaard <rkk@bankdata.dk>

* ⬆️ Bump ssri from 6.0.1 to 6.0.2 (#2027)

Bumps [ssri](https://github.com/npm/ssri) from 6.0.1 to 6.0.2.
- [Release notes](https://github.com/npm/ssri/releases)
- [Changelog](https://github.com/npm/ssri/blob/v6.0.2/CHANGELOG.md)
- [Commits](npm/ssri@v6.0.1...v6.0.2)

---
updated-dependencies:
- dependency-name: ssri
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: RasmusKjeldgaard <rkk@bankdata.dk>

* 📝 Remove grid from navigation (#2023)

This is to avoid new consumers of Kirby taking grid into use.

Co-authored-by: RasmusKjeldgaard <rkk@bankdata.dk>

* Add rounded corners and shadow to list (#2029)

* 💄 Introduce mixins for shadow and rounded corners

* 💄 Apply rounded corners and shadow to list

* 📝  Create list-experimental showcase

* 💄 Rework and rename mixins and global classes

* 💄 Enable first/last item padding in list

* 📝  Add examples for declarative list

* 📝  Refactor examples for use in showcase

* 📝  Correct attribute for section header examples

* 📝 Add examples to showcase

* ♻️ Use section instead of ion-list

Ion-list was not used for any specfic reason, and did not add
any necessary functionality, so it has been exchanged for a section
for now.

* 📝 Improve documentation

* ♻️ Rename slot for header to be more generic

* ✅ Add tests for list-experimental

* ✅  Remove focus flag from list-experimental suite

* ✅  Add tests for padding top and bottom on item

* 📝  Use correct name for slot in showcase

* 📝 Add headings to example

* ✅  Remove focus flag from item test

* 📝  Improve wording and structure of showcase

* 📝  Add multiple sections description to showcase

* ✅  Use ' instead of " in test

* Combine width style props into one in example

Co-authored-by: Jesper Kaltoft Vind <git@jesperkaltoft.dk>

* 🐛  Fix scss style for list example

* 💄 Move utility classes to own list scss module

* 💄 Apply refactored style classes to list

* ✅  Remove unnecessary wrapper div from list

Co-authored-by: Mads Buchmann Frederiksen <mbf@prochimp.dk>
Co-authored-by: Jesper Kaltoft Vind <git@jesperkaltoft.dk>

* Added inline style for kirby-modal-footer (#1982)

* updated modal-footer component ts, scss and corresponding tests. Updated showcase component.

* corrected test

* rephrased description

* Change type inline/fixed

* removed f in fdescribe, refactor of hostbinding

* 🔖Bumping version to 5.1.0 (designsystem)

Co-authored-by: Tobias Secher <77673731+bdytos@users.noreply.github.com>
Co-authored-by: Tobias Secher <bdytos@bankdata.dk>
Co-authored-by: Mads Buchmann Frederiksen <mbf@prochimp.dk>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jesper Kaltoft Vind <git@jesperkaltoft.dk>
Co-authored-by: NikolajSkousen <85226992+NikolajSkousen@users.noreply.github.com>
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.

[Enhancement] Rename 'dataLabels' input property for Chart component
2 participants