[No QA] [BYOC] [Bulk Card Assignments] Release 1: Generic Table component#77788
Conversation
chrispader
changed the title
chrispader
force-pushed
the
byoc-bulk-card-assign-r1-generic-table-component
branch
from
7e7e275 to
f3165ff
Compare
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
🇪🇸 Requested spanish translations here on Slack (to be added to PR description checklist). |
ikevin127
force-pushed
the
byoc-bulk-card-assign-r1-generic-table-component
branch
from
64881af to
1eb24c2
Compare
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
For the generic components, they probably need to be documented better. It would be a little difficult for me to know how to use them if I was just seeing them for the first time and trying to figure them out.
I don't know how much work it's gonna take to really get these generic components "perfect", but it might be good to somehow label them that they aren't totally ready to use by the general public quite yet, until they are meeting all our new React rules.
| <Table | ||
| ref={tableRef} | ||
| data={data} | ||
| columns={columns} | ||
| renderItem={renderItem} | ||
| keyExtractor={keyExtractor} | ||
| compareItems={compareItems} | ||
| isItemInSearch={isItemInSearch} | ||
| isItemInFilter={isItemInFilter} | ||
| filters={filterConfig} | ||
| > | ||
| <View style={shouldShowNarrowLayout && styles.mb5}>{renderHeaderButtons?.(<Table.SearchBar />, <Table.FilterButtons />)}</View> | ||
|
|
||
| {!shouldShowNarrowLayout && <Table.Header />} | ||
|
|
||
| <Table.Body /> | ||
| </Table> |
There was a problem hiding this comment.
In order to really embrace some of our new React rules (particularly, the one about composition), I would expect to see this look more like:
<Table>
{renderHeaderButtons && (
<View style={shouldShowNarrowLayout && styles.mb5}>
<Table.Sort compareItems={compareItems} />
<Table.Filter isItemInFilter={isItemInFilter} filters={filterConfig} />
</View>
)}
<Table.Search isItemInSearch={isItemInSearch} />
<Table.Body data={data} columns={columns}>
<Table.Row renderItem={renderItem} keyExtractor={keyExtractor} />
</Table.Body>
</Table>
I'd be curious to get some thoughts from @adhorodyski on this approach
There was a problem hiding this comment.
I'm able to refactor this even more, so we don't need this pattern.
…extra keyExtractor
|
@tgolen @carlosmiceli this should be ready for another review! @ikevin127 added lots of documentation on how to use the Table component and also added JSDoc comments! Thanks @ikevin127 Note: Going to address improving this UI tomorrow morning |
|
🚧 @dylanexpensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
…-generic-table-component
chrispader
force-pushed
the
byoc-bulk-card-assign-r1-generic-table-component
branch
from
6250484 to
62f7946
Compare
| @@ -0,0 +1,249 @@ | |||
| # Table Component | |||
…-generic-table-component
tgolen
merged commit e859dff
into
Expensify:byoc-bulk-card-assign-r1
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.86-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.86-4 🚀
|
| ListEmptyComponent={isEmptySearchResult ? EmptySearchComponent : ListEmptyComponent} | ||
| contentContainerStyle={[filteredAndSortedData.length === 0 && styles.flex1, listContentContainerStyle, contentContainerStyle]} | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...listProps} |
There was a problem hiding this comment.
Coming from the #82363 checklist: when FlashList/ScrollView includes TableSearchBar with clear functionality, and the keyboard is open, we should support the default keyboardShouldPersistTaps="handled" to allow actions while the keyboard is visible.
7 participants
@tgolen @carlosmiceli
Explanation of Change
This PR implements a generic table component that accepts generic data, filters, sort and search configs. We will use this first for the
WorkspaceCompanyCardstable, but most of the UI and components were re-used from theSearchPage("Reports") table. This eventually can be used there as well.Fixed Issues
$ #77926
PROPOSAL:
Tests
None needed.
Offline tests
None needed.
QA Steps
None needed.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari