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

DS 835: Table Element #2548

Merged
merged 40 commits into from
Oct 18, 2022
Merged

DS 835: Table Element #2548

merged 40 commits into from
Oct 18, 2022

Conversation

MarcinMr
Copy link
Collaborator

@MarcinMr MarcinMr commented Sep 28, 2022

Jira

https://pegadigitalit.atlassian.net/browse/DS-835

Summary

The new element table is created that can replace the basic functionalities of the current Table Component

Details

  1. Much of the props and functionality are taken from the old table component, for example:
  • only rows table
  • table with top headers only
  • table with side headers only
  • table with both top and side headers
  • caption of the table
  • borderless table
  • format table (regular, numeric)
  1. The following methods of display
  • Twig (appearance and behavior can be achieved via choosing props from the schema)
  • HTML only (appearance and behavior can be achieved by adding appropriate classes to the <table>)
  • HTML only (without the tbody) (appearance and behavior can be achieved by adding appropriate classes to the <table>)
  1. JavaScript creates a wrapper and wraps the <table> if needed
  2. There are props that set the first, second, or first two columns as fixed width
  3. There is now a full width prop that will changed the default behavior (inline-block) to be full width
  4. There are sticky header functionality now included

How to test

(Focus on testing in all browsers)

  • Pull the branch.
  • Check the code, especially the CSS and JS.
  • Check the table-element docs.
  • Go to test folder and find table test examples.
  • Play with them and confirm they work properly.

Release notes

There is a new Table Element that replaces the basic functionalities of the current Table Component

@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Sep 28, 2022
@colbytcook colbytcook requested a deployment to feature/DS-835-table-basic--3b60904--commit-preview September 28, 2022 13:24 Abandoned
@colbytcook colbytcook requested a deployment to feature/DS-833-List-Element--bb57b43--commit-preview September 28, 2022 13:29 In progress
@colbytcook colbytcook temporarily deployed to feature/DS-835-table-basic--branch-preview October 3, 2022 19:53 Inactive
@colbytcook colbytcook requested a deployment to feature/DS-835-table-basic--c48936d--commit-preview October 4, 2022 14:23 Abandoned
@colbytcook colbytcook temporarily deployed to feature/DS-835-table-basic--branch-preview October 4, 2022 18:30 Inactive
@colbytcook colbytcook had a problem deploying to feature/DS-835-table-basic--branch-preview October 11, 2022 19:26 Failure
@colbytcook colbytcook temporarily deployed to feature/DS-835-table-basic--branch-preview October 17, 2022 20:59 Inactive
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@colbytcook, I reviewed the docs and noticed:

  • Docs > Table Examples: The full-width tables aren't full width.
  • Tests > Table Default html: The tables are full-width until the JS loads. I think @MarcinMr works around this on the 6.x branch by scoping the width: 100% rule to .c-bolt-table > table, or in your case e-bolt-table-wrapper > .e-bolt-table.

These are the only issues I found. Here are a couple other thoughts, not blockers:

Full-width tables in wysiwyg
If I'm a content author and I make a plain HTML table and I want it to be width 100% (and I'm not going to edit the raw markup), how do I do this? Since the "full-width" style comes from the wrapper and the wrapper is added by JS I don't think there's anything in place to support full-width tables for a non-power-user. To solve that we'd have to check for some additional class on the <table> itself. Is this a use case we are solving for? Any thoughts?

Wrapper full-width classname
To make the container full-width I'd actually go with u-bolt-block, like @MarcinMr did on the 6.x branch. I didn't realize that literally the only thing this wrapper had to do was set display: block. Alternatively, if you prefer a component class, maybe e-bolt-table-wrapper--full-width would tell the user more about what it does?

@colbytcook colbytcook temporarily deployed to feature/DS-835-table-basic--branch-preview October 17, 2022 21:39 Inactive
@colbytcook
Copy link
Contributor

@danielamorse I addressed the feedback you had

  • Docs > Table Examples: the full width tables now render full width
  • Tests > Table Default html: Updated the CSS to stop the flashes

Full Width Prop
I added a full_width prop into the schema that will do three things.

  1. It will add a e-bolt-table--full-width class to the <table>. This class will apply a width: 100%; to the table to help prevent flash.
  2. The table.twig will now apply a e-bolt-table-wrapper--full-width class to the e-bolt-table-wrapper that will make the wrapper display block (and forcing it to be full width).
  3. The table JS will now poll the <table> for the e-bolt-table--full-width class and add the e-bolt-table-wrapper--full-width class to the wrapper when it is being rendered.

For WYSIWYG support, we can simply add a class (e-bolt-table--full-width) to the table element to achieve the desired effect.

I updated the existing docs pages with these changes (where applicable) and created a new documentation page for the full width prop. Let me know if you have any other questions regarding this.

@colbytcook colbytcook temporarily deployed to feature/DS-835-table-basic--branch-preview October 18, 2022 00:48 Inactive
@danielamorse danielamorse self-requested a review October 18, 2022 01:05
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@colbytcook awesome, love the full_width prop. Tested locally and it looks good to me.

@colbytcook colbytcook requested a deployment to feature/DS-835-table-basic--4c08f54--commit-preview October 18, 2022 01:13 In progress
@colbytcook colbytcook temporarily deployed to feature/DS-835-table-basic--branch-preview October 18, 2022 01:20 Inactive
Copy link
Collaborator

@cjwhitedev cjwhitedev left a comment

Choose a reason for hiding this comment

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

  • i think the PR description needs to be updated again, at least since adding the full_width prop, if not more than that.
  • see inline comment as well

@@ -0,0 +1,114 @@
{% set usage %}{% verbatim %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this demo could probably be shortened, maybe like layout's. The other demos on the page show the full context.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cjwhitedev I don't mind this initial demo page being longer than Layouts because layout examples are larger scoped than a table.

@colbytcook
Copy link
Contributor

@cjwhitedev Updated the PR description and responded to your inline comment

@colbytcook colbytcook requested a review from cjwhitedev October 18, 2022 13:55
@colbytcook colbytcook merged commit 2eba50d into master Oct 18, 2022
@colbytcook colbytcook deleted the feature/DS-835-table-basic branch October 18, 2022 14:03
description:
'Generates a table cell as a table header &lt;th&gt; element if set to true. &lt;th&gt; elements can be used in table head, table body, or table footer.',
},
filters: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is filters a real prop? I don't see it used in the twig template or any of the examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants