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

fix: set style property priority using proper API method #12019

Merged
merged 3 commits into from
Nov 4, 2021

Conversation

denis-anisimov
Copy link
Contributor

@denis-anisimov denis-anisimov commented Oct 12, 2021

fixes #11981

Description

Please list all relevant dependencies in this section and provide summary of the change, motivation and context.

Fixes # (issue)

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/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

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@mshabarov mshabarov self-requested a review November 1, 2021 12:11
Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

@pleku the way how the important rule is identified in the input value string looks for me a bit tricky, but I don't know a better solution. The main idea behind, I believe, is to verify that the passed input contains a proper rule by parsing it through the test element and check the result of element.getStyle().getPropertyPriority(name). Unit tests and usage test are in place.
I'm not sure the line 754 is needed, but otherwise LGTM.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ pleku
❌ Denis Anisimov


Denis Anisimov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 2 issues

  1. CRITICAL SimpleElementBindingStrategy.java#L1486: Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule
  2. CRITICAL SimpleElementBindingStrategy.java#L1492: Either log or rethrow this exception. rule

@pleku pleku removed the request for review from miguelatvaadin November 4, 2021 07:17
@pleku pleku merged commit 24b8063 into master Nov 4, 2021
@pleku pleku deleted the 11981-stytle-priority branch November 4, 2021 07:18
vaadin-bot pushed a commit that referenced this pull request Nov 4, 2021
fixes #11981

Co-authored-by: Denis Anisimov <[email protected]>
Co-authored-by: Pekka Hyvönen <[email protected]>
vaadin-bot pushed a commit that referenced this pull request Nov 4, 2021
fixes #11981

Co-authored-by: Denis Anisimov <[email protected]>
Co-authored-by: Pekka Hyvönen <[email protected]>
vaadin-bot added a commit that referenced this pull request Nov 4, 2021
…2269)

fixes #11981

Co-authored-by: Denis Anisimov <[email protected]>
Co-authored-by: Pekka Hyvönen <[email protected]>

Co-authored-by: Denis <[email protected]>
Co-authored-by: Denis Anisimov <[email protected]>
Co-authored-by: Pekka Hyvönen <[email protected]>
vaadin-bot added a commit that referenced this pull request Nov 4, 2021
…2268)

fixes #11981

Co-authored-by: Denis Anisimov <[email protected]>
Co-authored-by: Pekka Hyvönen <[email protected]>

Co-authored-by: Denis <[email protected]>
Co-authored-by: Denis Anisimov <[email protected]>
Co-authored-by: Pekka Hyvönen <[email protected]>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.beta2 and is also targeting the upcoming stable 22.0.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element API Style doesn't work with CSS values with !important
5 participants