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!: rename createWebComponent method #19309

Merged
merged 3 commits into from
May 13, 2024
Merged

fix!: rename createWebComponent method #19309

merged 3 commits into from
May 13, 2024

Conversation

caalador
Copy link
Contributor

@caalador caalador commented May 6, 2024

Rename the createWebComponent function
as the create gives the impression that
the function is heavy.

Rename the createWebComponent function
as the create gives the impression that
the function is heavy.
Copy link

github-actions bot commented May 6, 2024

Test Results

1 103 files  ±0  1 103 suites  ±0   1h 28m 49s ⏱️ + 5m 2s
7 006 tests ±0  6 957 ✅ ±0  49 💤 ±0  0 ❌ ±0 
7 378 runs  +2  7 319 ✅ +2  59 💤 ±0  0 ❌ ±0 

Results for commit 8b68b0f. ± Comparison against base commit fa1ebd0.

♻️ This comment has been updated with latest results.

@@ -446,7 +446,7 @@ interface Properties {
* @param onload optional callback to be called for script onload
* @param onerror optional callback for error loading the script
*/
export const createWebComponent = (tag: string, props?: Properties, onload?: () => void, onerror?: (err:any) => void) => {
export const webComponentTSX = (tag: string, props?: Properties, onload?: () => void, onerror?: (err:any) => void) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is this limited to work with TSX only, or could it even be just short as webComponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it returns a React element and not a webComponent. Another that was was getWebComponent, but that also feels missleading.

Should it actually be flowReactElement? This would not mix in webComponent into the name as it would explicitly name what is done.

Copy link
Member

Choose a reason for hiding this comment

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

or just reactElement then. It's imported from a file called Flow, so context is already known by the caller.

Copy link
Contributor

Choose a reason for hiding this comment

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

reactElement sounds good to me as well.
TSX postfix is not needed IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed.

TSX was only for if it was webComponent so you would know what to expect as you are not getting a webComponent.

@mshabarov
Copy link
Contributor

This has a chance to be included into 24.4 for better DX, so I've added the cherry-pick label for 24.4.

@caalador caalador marked this pull request as ready for review May 10, 2024 06:59
@mshabarov
Copy link
Contributor

Let's wait for another opinions for a while.

caalador added a commit to vaadin/docs that referenced this pull request May 10, 2024
Updated documentation to use the
new method name after refactor in
vaadin/flow#19309
Copy link

sonarcloud bot commented May 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@marcushellberg
Copy link
Member

I like reactElement, simple and clear.

@mshabarov mshabarov merged commit 4cb7723 into main May 13, 2024
26 checks passed
@mshabarov mshabarov deleted the dx/rename-method branch May 13, 2024 06:46
vaadin-bot pushed a commit that referenced this pull request May 13, 2024
* fix!: rename createWebComponent method

Rename the createWebComponent function
as the create gives the impression that
the function is heavy.

* renameMethod to reactElement

---------

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
vaadin-bot added a commit that referenced this pull request May 13, 2024
* fix!: rename createWebComponent method

Rename the createWebComponent function
as the create gives the impression that
the function is heavy.

* renameMethod to reactElement

---------

Co-authored-by: caalador <mikael.grankvist@vaadin.com>
Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
mshabarov added a commit to vaadin/docs that referenced this pull request May 15, 2024
Updated documentation to use the
new method name after refactor in
vaadin/flow#19309

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.alpha1 and is also targeting the upcoming stable 24.5.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.

None yet

5 participants