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(Table): new component #1129

Merged
merged 22 commits into from
Jun 5, 2024
Merged

feat(Table): new component #1129

merged 22 commits into from
Jun 5, 2024

Conversation

atabel
Copy link
Collaborator

@atabel atabel commented May 24, 2024

jira: WEB-1870

Copy link

github-actions bot commented May 24, 2024

Size stats

master this branch diff
Total JS 10.7 MB 10.7 MB +10.8 kB
JS without icons 1.94 MB 1.95 MB +10.8 kB
Lib overhead 53.8 kB 53.8 kB 0 B
Lib overhead (gzip) 14.4 kB 14.4 kB 0 B

Copy link

github-actions bot commented May 24, 2024

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

Copy link

github-actions bot commented May 24, 2024

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-2gkmfmlsn-flows-projects-65bb050e.vercel.app

Built with commit 6346e75.
This pull request is being automatically deployed with vercel-action

Copy link

github-actions bot commented May 24, 2024

Screenshot tests report

✔️ All passing

@@ -23,7 +23,7 @@ const NavigationBarTest = (): JSX.Element => (
</Badge>
</NavigationBarAction>
<NavigationBarAction onPress={() => {}} aria-label="Open profile">
<Avatar size={24} initials="ML" src="https://source.unsplash.com/600x600/?face" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unrelated change, fixed this test because it was using an external image and it was failing to load

src/table.tsx Show resolved Hide resolved
src/table.tsx Outdated
import type {DataAttributes} from './utils/types';

type TableProps = {
heading: Array<string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you confirm if headers also support custom elements @aweell ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not according the spec, and I think it could be problematic, how would you show them in mobile (collapse-rows mode)?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, I remember talking to Alex about this but I don't remember what the outcome was. I thought that the custom item would be included and that in mobile it is the product's responsibility... that's why I wanted Alex's confirmation.

The product could use strings in mobile and custom composition in desktop, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was the idea yes, allow slots in headers mostly for desktop views. It was changed in a small paragraph in the desktop behaviour but not in the anatomy, is already fixed in the specs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


import type {DataAttributes} from './utils/types';

type TableProps = {
Copy link
Contributor

@yceballost yceballost May 27, 2024

Choose a reason for hiding this comment

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

missing alignItems prop to align content horizontally

image

name: 'Table',
code: `
<Table
heading={[
Copy link
Contributor

Choose a reason for hiding this comment

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

can you include columnTextAlign to make easy the usage of this prop? (also for boxed snippet)

columnTextAlign={["left", "right", "right", "right", "center", "right"]}

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a wrong usage, but if you write a string in columnTextAlign prop, the alignment changes for each letter. That's weird and I don't know if it is something to avoid

columnTextAlign.-.Table.-.bug.mov

src/table.tsx Outdated Show resolved Hide resolved
@atabel atabel requested a review from Marcosld May 27, 2024 13:23
Copy link
Member

@pladaria pladaria left a comment

Choose a reason for hiding this comment

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

nice!
As commented in Teams, in mobile, when fullwidth, it would be great to have a way to bleed outside the responsive layout and set the padding/margin to the table. This way it would scroll over the whole viewport width

Copy link
Contributor

Choose a reason for hiding this comment

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

Visually stunning

src/table.tsx Outdated
columnWidth,
...otherProps
}: TableProps,
ref: React.Ref<HTMLDivElement>
) => {
const getColumnTextAlign = (column: number) => {
const defaultTextAlign = 'left';
if (columnTextAlign) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you can use a default parameter to simplify this to: return Array.isArray(columnTextAlign) ? columnTextAlign[column] : columnTextAlign

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not really, it whould be something like:

Array.isArray(columnTextAlign) ? columnTextAlign[column] ?? defaultTextAlign : columnTextAlign;

Copy link
Contributor

@Marcosld Marcosld May 28, 2024

Choose a reason for hiding this comment

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

Oh I see, that's in case the user just sends the textAlign for some of the columns... but given input parameter is an array and not something like a map[columnIdx->alignment]... is that a good use of the API? 🤔

src/table.tsx Show resolved Hide resolved
Copy link
Contributor

@yceballost yceballost left a comment

Choose a reason for hiding this comment

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

osom 🤩

@Marcosld Marcosld self-requested a review May 30, 2024 15:35
@atabel atabel requested a review from pladaria June 4, 2024 07:10
src/table.css.ts Outdated Show resolved Hide resolved
@atabel atabel requested a review from aweell June 4, 2024 13:19
@atabel atabel requested a review from marcoskolodny June 5, 2024 08:58
@atabel atabel removed the request for review from Marcosld June 5, 2024 12:42
@atabel atabel added this pull request to the merge queue Jun 5, 2024
Merged via the queue into master with commit 328e013 Jun 5, 2024
11 checks passed
@atabel atabel deleted the atoledano-table branch June 5, 2024 12:52
tuentisre pushed a commit that referenced this pull request Jun 19, 2024
# [15.11.0](v15.10.0...v15.11.0) (2024-06-19)

### Bug Fixes

* **ButtonLayout:** add bleed when using only link ([#1150](#1150)) ([554f98a](554f98a))
* **Counter:** add aria-live to value ([#1146](#1146)) ([3e2e09b](3e2e09b))
* **Header:** bleed not working in o2-new skin ([#1137](#1137)) ([00fb632](00fb632))
* **MainNavigationBar:** remove logo space in mobile when no sections are given ([#1149](#1149)) ([e4c03a0](e4c03a0))
* **Select:** set text color in native version ([#1141](#1141)) ([eedf265](eedf265))
* **Spinner:** use controlActivatedInverse token as default when used inside inverse variant ([#1133](#1133)) ([38a192d](38a192d))

### Features

* **Cards:** improve accessibility ([#1139](#1139)) ([dde9cc5](dde9cc5))
* **Chip:** allow using badge in selectable chips ([#1134](#1134)) ([9ecda7c](9ecda7c))
* **Circle:** custom background support ([#1136](#1136)) ([bedeaa4](bedeaa4))
* **CoverHero:** new component ([#1144](#1144)) ([a655e6e](a655e6e))
* **DisplayMediaCard, PosterCard:** add extra ([#1131](#1131)) ([501cf73](501cf73))
* **EmptyState:** allow using only ButtonLink as action ([#1140](#1140)) ([d73c219](d73c219))
* **HighlightedCard:** support for alt for image ([#1135](#1135)) ([c9ba728](c9ba728))
* **Image:** Custom fallback icon in Vivo New ([#1145](#1145)) ([ec600fe](ec600fe))
* **Switch:** Improve animation ([#1142](#1142)) ([8162eed](8162eed))
* **Table:** new component ([#1129](#1129)) ([328e013](328e013))
* **Timer:** create component ([#1130](#1130)) ([0b3253e](0b3253e))
* **Touchable:** newTab support in to links ([#1143](#1143)) ([eff07e3](eff07e3))
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 15.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants