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

feat: deprecate Label and add NativeLabel as replacement #16708

Merged
merged 12 commits into from
May 16, 2023

Conversation

knoobie
Copy link
Contributor

@knoobie knoobie commented Apr 27, 2023

Description

Adds NativeLabel as direct replacement for the HTML-based Label component / element.

Fixes #4384
Fixes #3532

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.

@mcollovati mcollovati added the Contribution PRs coming from the community or external to the team label Apr 27, 2023
@github-actions
Copy link

github-actions bot commented May 8, 2023

Test Results

   990 files  +  1     990 suites  +1   1h 22m 20s ⏱️ - 9m 41s
6 270 tests +12  6 229 ✔️ +12  41 💤 ±0  0 ±0 
6 518 runs  +10  6 470 ✔️ +  8  48 💤 +2  0 ±0 

Results for commit 683ba1d. ± Comparison against base commit a23d76d.

♻️ This comment has been updated with latest results.

@tltv
Copy link
Member

tltv commented May 9, 2023

There are around 40 test classes in the flow repository alone where Label is used. Looking quickly many tests are using Label in a wrong way, meaning they should be really just Span or Div instead. However, as Label will be deprecated and NativeLabel is direct replacement, then all these tests should be changed to use NativeLabel and make sure tests still works.

@tltv tltv requested a review from mshabarov May 9, 2023 12:48
@mshabarov mshabarov marked this pull request as draft May 12, 2023 11:57
@mshabarov mshabarov changed the title feat!: deprecate Label and add NativeLabel as replacement feat: deprecate Label and add NativeLabel as replacement May 12, 2023
@knoobie knoobie marked this pull request as ready for review May 12, 2023 16:49
// devs. This should be dealt with by devs in development
// mode.
}
checkForAttributeOnAttach = attachEvent.getUI()
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that before adding a new listener by beforeClientResponse, it should check

if (checkForAttributeOnAttach == null) { 

otherwise I don't understand why we use beforeClientResponse at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mshabarov
Copy link
Contributor

mshabarov commented May 16, 2023

The build validation stops because of compilation errors:

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project flow-html-components: Compilation failure
Error:  flow/flow-html-components/src/main/java/com/vaadin/flow/component/html/Label.java:[184,33] cannot access jakarta.servlet.http.HttpSessionBindingListener
Error:    class file for jakarta.servlet.http.HttpSessionBindingListener not found

This pull request doesn't add/import this class, this is weird.

flow-html-components/pom.xml Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.1.0.

@knoobie
Copy link
Contributor Author

knoobie commented Jun 21, 2023

@mshabarov I just realized that there are a lot of usages within the flow-components that are still using the old Label and polluting the logs because they are not associated on the server side, but on the client side thanks to the slot=label Attribute. I'm wondering if we should add this condition also to the list of valid usages to reduce the amount of work the component team has to do urgendly to remove the log spam for users.

Example FormLayout or RadioGroup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution PRs coming from the community or external to the team Released with Vaadin 24.1.0 +1.0.0
Projects
None yet
6 participants