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: add raw html table and related components #18553

Merged
merged 32 commits into from
Jun 4, 2024
Merged

feat: add raw html table and related components #18553

merged 32 commits into from
Jun 4, 2024

Conversation

sosa-vaadin
Copy link
Contributor

@sosa-vaadin sosa-vaadin commented Jan 29, 2024

Description

Add component for <table> element and related components:

  • <caption>
  • <thead>
  • <tbody>
  • <tfoot>
  • <tr>
  • <th>
  • <td>

Fixes #15475

@mcollovati mcollovati added the Contribution PRs coming from the community or external to the team label Jan 29, 2024
Copy link

github-actions bot commented Jan 29, 2024

Test Results

1 124 files  +  9  1 124 suites  +9   1h 24m 6s ⏱️ -50s
7 240 tests +130  7 189 ✅ +130  51 💤 ±0  0 ❌ ±0 
7 596 runs  +119  7 535 ✅ +119  61 💤 ±0  0 ❌ ±0 

Results for commit 3b734c5. ± Comparison against base commit 135bcfe.

♻️ This comment has been updated with latest results.

@mshabarov
Copy link
Contributor

Before Flow team starts making the review, I'd like to ping @rolfsmeds and @yuriy-fix to ask if they have immediate complains against this feature in general, e.g. naming, design or the whole idea about having this component on Flow side.

@mshabarov mshabarov requested a review from tepi January 29, 2024 12:37
@tepi
Copy link
Contributor

tepi commented Jan 29, 2024

Not sure if I'm getting something wrong but seems to me this patch does not really provide any added value in terms of convenience APIs and testing facilities. Only convenience API I could find is adding some components to here and there within the table or its sub-components.

For example, comparing this to an add-on that was mentioned in the linked issue: HtmlTable in this patch vs Table in add-on.

Also, the testbench elements for Table and its parts are all empty.

I have a hard time understanding what exactly is the value this patch brings as such? Saves you from typing the tag names when creating the elements?

@sosa-vaadin
Copy link
Contributor Author

I focused on just adding bare bones components. I did not check the add-on and only based the components on other already existing html flow components. Since no API requirements or specifications were given in the issue (which I found only after I was done coding what is now in the PR) I submitted it as is.

I thought of adding a few convenience methods, but decided against it.

@yuriy-fix
Copy link
Contributor

I don't have any objections to proceeding with this contribution. My only suggestion would be to opt for Native prefix instead of Html for better alignment. I also agree with @tepi, as I don't see real benefit in providing empty classes without methods.

Regarding the component name Table (without prefixes), we would like to preserve it for the DS.

@sosa-vaadin sosa-vaadin marked this pull request as draft February 5, 2024 07:57
@sosa-vaadin
Copy link
Contributor Author

I've added a bunch of methods to match most of the features provided in the add-on. I still need to write tests for those so once I'm done (next CF) I will change it back from draft.

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Feb 12, 2024
@vaadin-bot vaadin-bot added +0.1.0 and removed +1.0.0 labels Feb 14, 2024
Copy link

sonarcloud bot commented Feb 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@sosa-vaadin sosa-vaadin marked this pull request as ready for review May 17, 2024 17:36
Copy link
Contributor

@AlainaFaisal AlainaFaisal left a comment

Choose a reason for hiding this comment

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

The code logic seems sound and I could not find any discrepancies.

@tepi tepi enabled auto-merge (squash) June 4, 2024 15:50
Copy link

sonarcloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
313 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tepi tepi merged commit f4bf965 into main Jun 4, 2024
25 of 26 checks passed
@tepi tepi deleted the html-table branch June 4, 2024 16:02
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.

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.5.0 +0.1.0
Projects
Development

Successfully merging this pull request may close these issues.

Add html table component et al
7 participants