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(TableVirtualized): use react-virtualized components to create new table #783

Merged
merged 5 commits into from
Oct 10, 2019

Conversation

sun-tea
Copy link
Contributor

@sun-tea sun-tea commented Oct 4, 2019

  • Feature
  • Fix
  • Enhancement

Brief

As we want to add windowing to our Table with react-virtualized, we have to use its components.
To not break the existing Table, for now we are adding a new table comp next to the current one.
First column, header and footer are sticky. Tests are basic since hopefully this Table should merge with the current one. Also this virtualized table will evolve pretty quickly to follow the new features of the sales report we'll have to cover.

Related / Associated Jira Cards :

Requirements :

⚠️ Requires running yarn command.

Todo - Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.

@sun-tea sun-tea added the WIP Work in progress label Oct 4, 2019
@sun-tea sun-tea force-pushed the feat/BP-370 branch 8 times, most recently from d82ac0c to 376428a Compare October 9, 2019 12:37
@sun-tea sun-tea removed the WIP Work in progress label Oct 9, 2019
@sun-tea sun-tea requested review from spartDev, Artikodin, anglol and ArTiSTiX and removed request for spartDev October 9, 2019 12:43
Copy link
Contributor

@spartDev spartDev left a comment

Choose a reason for hiding this comment

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

They are some problems:

  • When the virtualized table is in full width:
    Screenshot 2019-10-09 at 14 58 52
  • What about the table scrolling in storybook
  • The Table story won't work anymore

@spartDev spartDev assigned sun-tea and unassigned spartDev Oct 9, 2019
@sun-tea sun-tea changed the title refactor(Table): use react-virtualized components feat(TableVirtualized): use react-virtualized components to create new table Oct 9, 2019
@sun-tea
Copy link
Contributor Author

sun-tea commented Oct 9, 2019

  • I'll try to fix the width problem (per the doc I had to chose between dynamic height and dynamic width for cell size measures) so the width is hardcoded unfortunately, hence your screenshot
  • The table is scrollable, but your Mac settings hide them by default (mine have 'always' toggled)
    Screenshot 2019-10-09 at 15 40 32

Screenshot 2019-10-09 at 15 41 27

- What do you mean `The Table story won't work anymore` ? Existing Table should not be impacted at all since I didn't make any change to it

Artikodin
Artikodin previously approved these changes Oct 9, 2019
Copy link
Contributor

@Artikodin Artikodin left a comment

Choose a reason for hiding this comment

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

Looks good for me 👍
There is just a scroll behaviour I don't like but it's due to react-visualized.
scroll_glitch
It's nothing just a tiny issue.

@sun-tea
Copy link
Contributor Author

sun-tea commented Oct 9, 2019

spartDev
spartDev previously approved these changes Oct 10, 2019
@spartDev spartDev added resolve conflicts There are conflicts blocking merge to master and removed need review labels Oct 10, 2019
@spartDev spartDev assigned sun-tea and unassigned spartDev Oct 10, 2019
@Artikodin Artikodin self-requested a review October 10, 2019 08:14
Artikodin
Artikodin previously approved these changes Oct 10, 2019
@sun-tea sun-tea dismissed stale reviews from Artikodin and spartDev via 52c15ef October 10, 2019 08:46
@sun-tea sun-tea removed the resolve conflicts There are conflicts blocking merge to master label Oct 10, 2019
@spartDev spartDev merged commit 5122a3f into master Oct 10, 2019
@spartDev spartDev deleted the feat/BP-370 branch October 10, 2019 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants