Skip to content

Feat/analytical table dnd #229

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

Merged
merged 13 commits into from
Nov 26, 2019
Merged

Feat/analytical table dnd #229

merged 13 commits into from
Nov 26, 2019

Conversation

andybessm
Copy link
Contributor

Thank you for your contribution! 👏

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

@andybessm andybessm requested a review from vbersch November 19, 2019 13:25
@netlify
Copy link

netlify bot commented Nov 19, 2019

Deploy preview for ui5-webcomponents-react ready!

Built with commit e57e690

https://deploy-preview-229--ui5-webcomponents-react.netlify.com

@CLAassistant
Copy link

CLAassistant commented Nov 21, 2019

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Nov 21, 2019

Codecov Report

Merging #229 into master will increase coverage by 0.33%.
The diff coverage is 67.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   71.93%   72.26%   +0.33%     
==========================================
  Files         159      160       +1     
  Lines        3492     3534      +42     
  Branches      611      615       +4     
==========================================
+ Hits         2512     2554      +42     
+ Misses        763      756       -7     
- Partials      217      224       +7
Impacted Files Coverage Δ
...nts/AnalyticalTable/hooks/useTableHeaderStyling.ts 100% <100%> (ø) ⬆️
...ages/main/src/components/AnalyticalTable/index.tsx 75% <100%> (+1.31%) ⬆️
.../components/AnalyticalTable/ColumnHeader/index.tsx 65.51% <33.33%> (-3.72%) ⬇️
...omponents/AnalyticalTable/ColumnHeader/Resizer.tsx 41.93% <33.33%> (-0.93%) ⬇️
...components/AnalyticalTable/hooks/useDragAndDrop.ts 66.66% <66.66%> (ø)
...mponents/AnalyticalTable/hooks/useResizeColumns.ts 75% <75%> (-5%) ⬇️
...mponents/AnalyticalTable/defaults/Column/index.tsx 60% <0%> (-20%) ⬇️
...AnalyticalTable/virtualization/VirtualTableRow.tsx 65.21% <0%> (ø) ⬆️
...omponents/AnalyticalTable/hooks/useWindowResize.ts 80% <0%> (+5%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14c75e0...e57e690. Read the comment docs.

Copy link
Contributor

@vbersch vbersch left a comment

Choose a reason for hiding this comment

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

Works like a charm! Just some minor comments:

  • Could you please delete the package-lock.json files as we´re using yarn and its not a good idea to mix those up.
  • When I start to drag a column, but don´t drop it in a new position, the visual feedback that the column is being dragged does not disappear

@andybessm andybessm requested a review from vbersch November 25, 2019 08:50
Copy link
Contributor

@vbersch vbersch left a comment

Choose a reason for hiding this comment

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

Looks like we´re almost done!
Just two minor things:

  • There is a typo in the useDragAndDrop custom hook file name
  • Could you please add an onColumnsReordered event to the Analytical Table, so that someone can react from outside when columns are being reordered? The user should get the columns array in the new order as well as the column that has been moved.

@MarcusNotheis MarcusNotheis merged commit b208822 into master Nov 26, 2019
@MarcusNotheis MarcusNotheis deleted the feat/analytical-table-dnd branch November 26, 2019 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants