Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

feat(IM-5878): consider scroll position when taking snapshot #520

Closed
wants to merge 3 commits into from

Conversation

Donya-qlik
Copy link
Contributor

@Donya-qlik Donya-qlik commented Jan 21, 2024

What is the improvement ?:
Pivot table snapshot should consider the scroll position so WYSIWYG as the result.

How it is improved:
The idea is when the snapshot is taken, snapshotData content provides one more property rowPartialScrollHeight before the table re-renders. The considered property is basically a calculation for finding the hidden part of the table row when the scroll is positioned and add the result to the scrollTop/Left values when the snapshot is used.

Before merging this PR, all get-view-state properties will be reset to 0 to ensure they do not affect the current snapshot state. This will remain the case until further calculations are added

The attachment shows how the pv table looks like when the row partial height is applied on the snapshot.
Skärmbild 2024-01-22 004022

Comment on lines 6 to 10
rowPartialHeight: 125,
visibleTopIndex: 5,
visibleRows: 16,
page: 0,
rowsPerPage: 0,
Copy link
Contributor Author

@Donya-qlik Donya-qlik Jan 21, 2024

Choose a reason for hiding this comment

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

Hard coded for testing the scrolled table snapshot.
I think only the rowPartialHeight is considered for snapshot, so I'll remove the rest of properties for this PR. But they will be probably needed later for export improvement.

useMemo(
() => createViewService(layout.snapshotData),
// eslint-disable-next-line react-hooks/exhaustive-deps
[layout.snapshotData, pageInfo.page],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure pageInfo.page should be in the dependencies. It was there before, so I kept it as it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

pageInfo.page in deps means that the view service is "reset" or re-created when page changes. Which is what we want since all states in viewService are based on the current page.

@Donya-qlik Donya-qlik changed the title feat: get a viewState of pvTable when snapshot is taken feat(IM-4976): get a viewState of pvTable when snapshot is taken Jan 22, 2024
@Donya-qlik Donya-qlik changed the title feat(IM-4976): get a viewState of pvTable when snapshot is taken feat: get a viewState of pvTable when snapshot is taken Jan 22, 2024
src/ext/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Donya-qlik Donya-qlik left a comment

Choose a reason for hiding this comment

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

feat: change the feature flag

@Donya-qlik Donya-qlik changed the title feat: get a viewState of pvTable when snapshot is taken feat(IM-5878): consider scroll position when taking snapshot Jan 22, 2024
const element = verticalScrollableContainerRef.current;
if (element) {
// element.scrollLeft = scrollLeft;
element.scrollTop = scrollTopPartially;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely need to add a similar property for scrolLeft and other conditions if there are more, but maybe we can focus on vertical scroll for this PR

@enell enell closed this Jan 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants