feat: Migrate Table to fiori3 #1042

JKMarkowski · 2020-05-20T15:28:15Z

Related Issue

Related to #914

Descriptions

There are included

Base Table Component - Header, Body, Footer + sizes
Interactive states(hover, active, selected)
Table with checkboxes + sizes
Highlight Row
Table with pagination
Responsive table + pop-in mode
Filter / Sort of table
Navigation indicator inside table
Fixed Column

Breaking Changes

Added Class:
Modifiers:

fd-fable + (__header/__body/__cell) + (--no-vertical-borders/--no-horizontal-borders)
fd-table + (__row/_cell) + (--activable/--hoverable)
fd-table__cell--checkbox
fd-table__cell + (--success/--error/--warning--information)
fd-table__cell--status-indicator+ (--success/--error/--warning--information)

Responsive:

fd-table__row--main
fd-table__row--secondary

Removed Classes:
Elements:

fd-table__sort-column(--asc/--desc)
fd-table__context-menu
fd-table__context-menu-label
Modifiers:
fd-table__row(--success/--error/--warning/--information)

Screenshots

NOTE: If you've made any style changes, please provide appropriate screenshots (before and after) to help reviewers.

To see examples of which screenshots to include, go to Screenshot Examples.

Before:

After:

Please check whether the PR fulfills the following requirements

all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-styles/wiki/PR-Review-Checklist

netlify · 2020-05-20T15:32:02Z

Deploy preview for fundamental-styles ready!

Built with commit 0765802

https://deploy-preview-1042--fundamental-styles.netlify.app

mikerodonnell89

The entire table should have an outer border, 1px solid @sapUiListBorderColor

mikerodonnell89 · 2020-05-21T19:26:00Z

Too many cells in the footer on this example

mikerodonnell89 · 2020-05-21T19:33:28Z

Strange focus outline for condensed checkboxes

mikerodonnell89 · 2020-05-21T19:39:27Z

Table cell contents aligned left in RTL

mikerodonnell89 · 2020-05-21T20:09:19Z

src/table.scss

-  --fd-table-header-background-color: var(--fd-color-neutral-1);
+
+  $fd-table-cell-padding: 0.5rem;
+  $fd-table-header-height: 2.75rem;


Weird name for these variables because they are only used in cell selectors

Naming changed

InnaAtanasova

I don't think the cells should have active state. They have hover, selected, hover+selected but didn't see the active (I might have missed it). The cells get focus tho (seen in the demo app)
The RTL support is "missing" - the text should not be left-aligned and the first column padding (1rem) should go to the other side
Items that are navigatable have different color

Specs:

Freeze - shouldn't we have a horizontal/vertical scroll when we freeze a certain row or column? For example, if we freeze the first column, we will be able to horizontally scroll in order to see all the columns (this is usually when we can't fit all the columns in the vw)

InnaAtanasova · 2020-05-21T21:04:56Z

src/table.scss

+    padding: 0 $fd-table-cell-padding;
+    font-family: var(--sapFontFamily);
+    font-size: var(--sapFontSize);
+    font-weight: normal;


normal font-weight is coming with the fd-reset

InnaAtanasova · 2020-05-21T21:06:07Z

src/table.scss


-  &__row {
-    transition: background-color $fd-table-transition-params;
+  &__header {


I think your header is missing the border

InnaAtanasova · 2020-05-21T21:08:33Z

src/table.scss

+    font-size: var(--sapFontSize);
+    font-weight: normal;
+    color: inherit;
+    vertical-align: middle;


I don't think this is doing anything

JKMarkowski · 2020-05-22T12:11:14Z

Hi @InnaAtanasova

I don't think the cells should have active state. They have hover, selected, hover+selected but didn't see the active (I might have missed it). The cells get focus tho (seen in the demo app)

There is down state, which I understand as a active. You can take a look at activable cells/rows example on preview.

The RTL support is "missing" - the text should not be left-aligned and the first column padding (1rem) should go to the other side

Fixed

Items that are navigatable have different color

It's shown only on some images, I don't know what to do with links actually links got own colors. Maybe I should have something like fd-table__text to keep color inherited.

Freeze - shouldn't we have a horizontal/vertical scroll when we freeze a certain row or column? For example, if we freeze the first column, we will be able to horizontally scroll in order to see all the columns (this is usually when we can't fit all the columns in the vw)

We have Fixed Column now. It's in last example, I would also add something like fixed row.

The rest is fixed. I think you had a little bit outdated documentation.

salarenko · 2020-05-22T12:50:09Z

Nice work.
I'm not sure if this composition approach is correct in terms of interactivity and other features. Solid base should be a table with basic styles. Currently we bring with base table additional behaviors like:

Marking row on hover
Interactive table headers

I'd rather consider those as extensions of basic table.

prsdthkr · 2020-05-23T01:29:34Z

Great work :D
I noticed that the table here behaves like a responsive table. If we were to follow the spec for the grid table, the columns will not be responsive by default as seen in this example. Instead, a scrollable view is provided (in smaller viewports or at 200% zoom). Also there are 3 modes for column width behavior as seen here: static, flexible, and mixed.

I would understand if this is out of scope for this PR. Other than that LGTM.

JKMarkowski · 2020-05-28T11:24:34Z

Hi @prsdthkr,

Those features with responsive behaviour are up to application specification. If user adds some min-width to cells and adds wrapper with overflow-x it's possible to achieve. I added example with fixed column, which actually shows how it can be used.

Also there is responsive table example with pop-in mode, which changes view of table(it has modified markup a little bit)

I'm not sure what about the static, flexible, and mixed. It's not mentioned anywhere in fiori3 specs. We can add this example with dummy cells in future. Thanks for taking a look at it.

InnaAtanasova · 2020-05-28T19:24:53Z

Documentation:
The checkbox needs to be checked with selected state

Responsive table:

missing border
prob not achievable with pure css but the whole item should get hover and selected state:

Navigation:

I think the button should not have states. The states should be applied to the cell

InnaAtanasova · 2020-05-28T19:26:29Z

src/table.scss

+      width: 2rem;
+      min-width: 2rem;


should be put in a variable

InnaAtanasova · 2020-05-28T19:27:15Z

src/table.scss

-        }
-      }
+    &--checkbox {
+      width: 10px;


stefanoScalzo · 2020-06-01T14:11:01Z

The toolbar border is overlapping with the table border

JKMarkowski · 2020-06-01T14:11:53Z

@stefanoScalzo Should It be considered as an issue?

stefanoScalzo · 2020-06-01T14:23:10Z

@stefanoScalzo Should I be considered as issue?

True, something to discuss because I think we need to give our components the option to not include the border for cases like this? either that or we use a class that positions the elements lower with a smaller z index to hide it, but this may not work due to some using box-shadow. We need to discuss it.

stefanoScalzo · 2020-06-01T14:41:32Z

For: Responsive Table - Pop-in Mode with Checkboxes
When clicking on the checkbox the whole table gets highlighted but when clicking the table nothing gets checked.

JKMarkowski · 2020-06-01T14:43:34Z

For: Responsive Table - Pop-in Mode with Checkboxes
When clicking on the checkbox the whole table gets highlighted but when clicking the table nothing gets checked.

there is no way to make it work on pure css, I can't wrap row/cell to label.

stefanoScalzo · 2020-06-01T14:53:53Z

Can you include Navigated Item Indicator Bar in your responsive table example please

stefanoScalzo · 2020-06-01T15:10:12Z

Overflow property not working properly

mikerodonnell89 · 2020-06-02T14:03:17Z

Filter section has compact buttons but the rest of the table is cozy - intentional?

JKMarkowski · 2020-06-02T14:14:12Z

@mikerodonnell89 It's done by accident, I will change it

…r example table

InnaAtanasova · 2020-06-02T14:59:28Z

Responsive table:

missing border

It shouldn't have border

Sorry, I meant the border on the right, half of it has a border, the other half doesn't

InnaAtanasova · 2020-06-02T15:06:07Z

Impressive job with the table! Once Travis is happy you can go ahead and merge it

salarenko · 2020-06-03T09:05:21Z

Looks good. I'm not sure about one visual thing. Shouldn't the checkbox in collapsed table be aligned to the top? The docs say nothing about it, but thats the case @InnaAtanasova had to cover in very similar List with checkbox layout and it looks much better.

This is how it looks now:

This is how it looks with checkbox aligned to the top: (vertical-align)

mikerodonnell89 mentioned this pull request May 21, 2020

Fiori 3 Table #914

Closed

6 tasks

JKMarkowski force-pushed the feat/tables-fiori3 branch from c366982 to 950b0f8 Compare May 21, 2020 12:03

JKMarkowski changed the title ~~WIP feat/tables fiori3~~ feat: Migrate Table to fiori3 May 21, 2020

droshev requested a review from a team May 21, 2020 16:30

droshev assigned JKMarkowski May 21, 2020

droshev added Fiori 3 refactoring labels May 21, 2020

droshev added this to the Sprint 37 - Shenzhen milestone May 21, 2020

mikerodonnell89 requested changes May 21, 2020

View reviewed changes

mikerodonnell89 reviewed May 21, 2020

View reviewed changes

InnaAtanasova reviewed May 21, 2020

View reviewed changes

JKMarkowski requested review from mikerodonnell89 and InnaAtanasova May 22, 2020 12:52

droshev added the table label May 22, 2020

droshev requested a review from a team May 22, 2020 20:30

rengare approved these changes May 25, 2020

View reviewed changes

Vanessa-Cusmich modified the milestones: Sprint 37 - Shenzhen, Sprint 38 - Seoul May 25, 2020

InnaAtanasova reviewed May 28, 2020

View reviewed changes

src/table.scss Outdated

}

}

&--checkbox {

width: 10px;

Copy link

Contributor

InnaAtanasova May 28, 2020

Choose a reason for hiding this comment

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

rem value

JKMarkowski reacted with thumbs up emoji

JKMarkowski requested a review from InnaAtanasova June 1, 2020 11:22

Add documentation, add advanced toolbar example

6305ff9

Fix overflow, add navigation indicator example

beab6bc

Remove interactive state, when with checkboxed, put compact to toolba…

2170b47

…r example table

JKMarkowski and others added 3 commits June 2, 2020 17:21

Add border to last cell on popin

6323994

add storybook and visual tests templates

a8dff18

add visual tests

458587d

mikerodonnell89 approved these changes Jun 2, 2020

View reviewed changes

JKMarkowski requested a review from salarenko June 3, 2020 08:30

salarenko approved these changes Jun 3, 2020

View reviewed changes

Place checkboxes at the top

0765802

JKMarkowski merged commit c068511 into master Jun 3, 2020

JKMarkowski deleted the feat/tables-fiori3 branch June 3, 2020 09:57

jbadan mentioned this pull request Jun 3, 2020

Table: implement new states/styles SAP/fundamental-react#1076

Open

This was referenced Aug 31, 2020

'High Contrast White' theme Table Header contrast #1037

Closed

'High Contrast Black' theme Table same text and background color #1017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Migrate Table to fiori3 #1042

feat: Migrate Table to fiori3 #1042

JKMarkowski commented May 20, 2020 •

edited by InnaAtanasova

Loading

netlify bot commented May 20, 2020 •

edited

Loading

mikerodonnell89 left a comment

mikerodonnell89 commented May 21, 2020 •

edited

Loading

mikerodonnell89 commented May 21, 2020 •

edited

Loading

mikerodonnell89 commented May 21, 2020

mikerodonnell89 May 21, 2020 •

edited

Loading

JKMarkowski May 22, 2020

InnaAtanasova left a comment

InnaAtanasova May 21, 2020

InnaAtanasova May 21, 2020

InnaAtanasova May 21, 2020

JKMarkowski commented May 22, 2020 •

edited

Loading

salarenko commented May 22, 2020 •

edited

Loading

prsdthkr commented May 23, 2020 •

edited

Loading

JKMarkowski commented May 28, 2020

InnaAtanasova commented May 28, 2020

InnaAtanasova May 28, 2020

InnaAtanasova May 28, 2020

stefanoScalzo commented Jun 1, 2020

JKMarkowski commented Jun 1, 2020 •

edited

Loading

stefanoScalzo commented Jun 1, 2020

stefanoScalzo commented Jun 1, 2020

JKMarkowski commented Jun 1, 2020

stefanoScalzo commented Jun 1, 2020

stefanoScalzo commented Jun 1, 2020

mikerodonnell89 commented Jun 2, 2020

JKMarkowski commented Jun 2, 2020

InnaAtanasova commented Jun 2, 2020

InnaAtanasova commented Jun 2, 2020

salarenko commented Jun 3, 2020

feat: Migrate Table to fiori3 #1042

feat: Migrate Table to fiori3 #1042

Conversation

JKMarkowski commented May 20, 2020 • edited by InnaAtanasova Loading

Related Issue

Descriptions

Breaking Changes

Screenshots

Before:

After:

Please check whether the PR fulfills the following requirements

netlify bot commented May 20, 2020 • edited Loading

mikerodonnell89 left a comment

Choose a reason for hiding this comment

mikerodonnell89 commented May 21, 2020 • edited Loading

mikerodonnell89 commented May 21, 2020 • edited Loading

mikerodonnell89 commented May 21, 2020

mikerodonnell89 May 21, 2020 • edited Loading

Choose a reason for hiding this comment

JKMarkowski May 22, 2020

Choose a reason for hiding this comment

InnaAtanasova left a comment

Choose a reason for hiding this comment

InnaAtanasova May 21, 2020

Choose a reason for hiding this comment

InnaAtanasova May 21, 2020

Choose a reason for hiding this comment

InnaAtanasova May 21, 2020

Choose a reason for hiding this comment

JKMarkowski commented May 22, 2020 • edited Loading

salarenko commented May 22, 2020 • edited Loading

prsdthkr commented May 23, 2020 • edited Loading

JKMarkowski commented May 28, 2020

InnaAtanasova commented May 28, 2020

InnaAtanasova May 28, 2020

Choose a reason for hiding this comment

InnaAtanasova May 28, 2020

Choose a reason for hiding this comment

stefanoScalzo commented Jun 1, 2020

JKMarkowski commented Jun 1, 2020 • edited Loading

stefanoScalzo commented Jun 1, 2020

stefanoScalzo commented Jun 1, 2020

JKMarkowski commented Jun 1, 2020

stefanoScalzo commented Jun 1, 2020

stefanoScalzo commented Jun 1, 2020

mikerodonnell89 commented Jun 2, 2020

JKMarkowski commented Jun 2, 2020

InnaAtanasova commented Jun 2, 2020

InnaAtanasova commented Jun 2, 2020

salarenko commented Jun 3, 2020

JKMarkowski commented May 20, 2020 •

edited by InnaAtanasova

Loading

netlify bot commented May 20, 2020 •

edited

Loading

mikerodonnell89 commented May 21, 2020 •

edited

Loading

mikerodonnell89 commented May 21, 2020 •

edited

Loading

mikerodonnell89 May 21, 2020 •

edited

Loading

JKMarkowski commented May 22, 2020 •

edited

Loading

salarenko commented May 22, 2020 •

edited

Loading

prsdthkr commented May 23, 2020 •

edited

Loading

JKMarkowski commented Jun 1, 2020 •

edited

Loading