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

[atable] fix for modal positioning #235

Merged
merged 7 commits into from
Jan 15, 2025
Merged

[atable] fix for modal positioning #235

merged 7 commits into from
Jan 15, 2025

Conversation

crabinak
Copy link
Collaborator

@Alchez @agritheory related to the issue we were having with the amodal not positioning correctly. This fix changes the modal position to fixed while still auto-detecting edges (so it doesn't get cropped off-screen). The position of the modal uses the position of the cell instead of offset positioning, so it shouldn't matter what the parent is doing (I used some test wrapping divs as parents to ATable and gave them various positioning).

Let me know if this resolves that weird positioning issue that was occurring.

Example image: The modal right aligns with the page since it would otherwise overflow and get cropped.
fix-modal-position

@crabinak crabinak requested review from Alchez and agritheory January 10, 2025 16:33
@Alchez Alchez changed the title Fix for modal positioning [atable] fix for modal positioning Jan 13, 2025
Copy link
Contributor

github-actions bot commented Jan 13, 2025

Coverage Report for ./atable

Status Category Percentage Covered / Total
🔴 Lines 67.08% (🎯 70%) 161 / 240
🔴 Statements 68.11% (🎯 70%) 173 / 254
🟢 Functions 79.59% (🎯 70%) 39 / 49
🔴 Branches 46.23% (🎯 70%) 86 / 186
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
atable/src/components/ACell.vue 90.38% 62.5% 100% 90.19% 101, 124-127, 157, 165-167
atable/src/components/ATable.vue 55.55% 35.29% 81.81% 54.71% 107-111, 121-149, 163-164
atable/src/components/ATableModal.vue 46.66% 20% 50% 42.85% 22-45, 49
Unchanged Files
atable/src/utils.ts 100% 100% 100% 100%
atable/src/components/AExpansionRow.vue 0% 0% 0% 0% 33-3
atable/src/components/ARow.vue 64.28% 56.25% 75% 58.33% 51-65, 17
atable/src/components/ATableHeader.vue 100% 57.14% 100% 100%
atable/src/stores/table.ts 74% 50% 84.21% 73.91% 39-40, 53, 84-85, 107-111, 127, 134, 138-149, 176, 184-185, 191
Generated in workflow #547 for commit 906ab22 by the Vitest Coverage Report Action

@Alchez
Copy link
Collaborator

Alchez commented Jan 13, 2025

@crabinak I think the logic for rendering modals for the last few cells has maybe regressed. Here's what I see in the ATable default story

image

image

@crabinak
Copy link
Collaborator Author

@Alchez I added a line to include the Y position of the window scroll when calculating the maximum height. Let me know if this fixes it now.

Copy link
Collaborator

@Alchez Alchez left a comment

Choose a reason for hiding this comment

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

@crabinak This looks good. But I've added a commit based on some typing warnings in the IDE. Can you please check out the change locally and verify that the behaviour hasn't changed?

@crabinak
Copy link
Collaborator Author

@Alchez It is still working fine. Thanks!

@crabinak crabinak marked this pull request as ready for review January 15, 2025 14:12
@Alchez Alchez merged commit cf7faf8 into development Jan 15, 2025
4 of 5 checks passed
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.

2 participants