Skip to content
This repository has been archived by the owner on Jul 21, 2021. It is now read-only.

Olivero-3093249: Implement tables on Olivero proof of concept. #18

Merged
merged 11 commits into from Jan 3, 2020

Conversation

proeung
Copy link
Contributor

@proeung proeung commented Nov 13, 2019

Olivero-3093249: Implement tables on Olivero proof of concept.

What kind of change does this PR introduce? (Bug fix, feature...)

  • Feature
  • Add a markup and styling of the table component.
  • The table is fully responsive and turns horizontal scroll on narrow breakpoints.

Additional Info

  • This PR is missing a markup for the sorting carrot functionality.
  • This is what we'll need to account for when we start the Drupal theming work.

index.html Show resolved Hide resolved
src/css/components/table.css Outdated Show resolved Hide resolved
src/css/components/table.css Outdated Show resolved Hide resolved
td,
th {
padding: var(--sp1-5) var(--sp1-5) var(--sp1-5) 0;
text-align: left; /* LTR */

Choose a reason for hiding this comment

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

We're actually gonna merge a PostCSS plugin that does the RTL stuff. So don't worry about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know!

src/css/components/table.css Outdated Show resolved Hide resolved
src/css/components/table.css Show resolved Hide resolved
src/css/components/table.css Show resolved Hide resolved
@proeung
Copy link
Contributor Author

proeung commented Nov 14, 2019

@mherchel Just pushed out the changes. Can you take a look again?

@mherchel
Copy link

Yeah, the grid changes broke the layout in FF. FF handles grid overflows differently. There's a couple more issues.

Screen Shot 2019-11-14 at 4 39 19 PM

@mherchel
Copy link

image

@proeung
Copy link
Contributor Author

proeung commented Nov 14, 2019

@mherchel With the layout issue within FF, may I suggest that instead of having .text-content be the class that wraps all of the elements. We instead set the grid column per tag so that we can easily align the table without to do negative margins or set width that bigger than the wrapper container or have some sort of auto-sizing columns.

@mherchel
Copy link

I think that might be impossible. 1) We won't be able to float images (and have text wrap) and 2) We can't even guarantee that people will be using tags.

@proeung
Copy link
Contributor Author

proeung commented Nov 18, 2019

@mherchel Please see below for the latest changelog made to the PR.

11/20 - Adjust the look and feel of the table <caption> based on the slack conversation with Jen. See the commit here - f0af226


As for the odd right padding issue that was seen last week, I realized that it was caused by the display: block property (see attached). This is needed for the background gradient to align correctly and for the horizontal scroll to work. Depending on the character count that's set the table row, it will expand to the 100% width. Anywho, I'm going to see if there's another solution to this issue, but in the meantime let me know if you have any idea.

Screen_Shot_2019-11-18_at_11_44_37_AM

@proeung
Copy link
Contributor Author

proeung commented Nov 22, 2019

@mherchel I've cleaned up the table.css file and added some placeholder content per our slack call. Can you take a look? Thanks!

@proeung proeung mentioned this pull request Nov 22, 2019
@proeung
Copy link
Contributor Author

proeung commented Nov 25, 2019

@mherchel Just added some adjustments to the linear-gradient direction based on the [dir] attribute. Regardless, here's the latest preview that's ready for your review - https://deploy-preview-18--olivero-poc.netlify.com/.

@proeung
Copy link
Contributor Author

proeung commented Dec 30, 2019

@mherchel When you have a chance, can you take a look at the changes made in this PR? Thanks!

@proeung
Copy link
Contributor Author

proeung commented Jan 3, 2020

@mherchel I'm going to merge this down to master for now. Please create follow up tickets for issues that you see related to the table. Thanks!

@proeung proeung merged commit b529c4f into master Jan 3, 2020
@proeung proeung deleted the 3093249-tables branch January 3, 2020 15:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants