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

[DT-1209] Abstract out the modal #1755

Merged
merged 8 commits into from
Feb 13, 2025

Conversation

s-rubenstein
Copy link
Contributor

No description provided.

@s-rubenstein s-rubenstein requested a review from a team as a code owner February 12, 2025 18:39
@s-rubenstein s-rubenstein requested review from rushtong and fboulnois and removed request for a team February 12, 2025 18:39
@s-rubenstein
Copy link
Contributor Author

Functionally this should be equivalent to existing code - the only change is code structure wise.

Copy link

cypress bot commented Feb 12, 2025

jade-data-repo-ui    Run #4088

Run Properties:  status check passed Passed #4088  •  git commit 8d293c657a ℹ️: Merge c49376f6c13af700eef158b70555f255d5420140 into b0d7bf1c1928459002cc2c99919d...
Project jade-data-repo-ui
Branch Review sr/dt-1209-abstract-full-view-modal
Run status status check passed Passed #4088
Run duration 03m 00s
Commit git commit 8d293c657a ℹ️: Merge c49376f6c13af700eef158b70555f255d5420140 into b0d7bf1c1928459002cc2c99919d...
Committer Sky Rubenstein
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 17
View all changes introduced in this branch ↗︎

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

One suggestion:

src/components/common/FullViewSnapshotModal.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

Thank you, looks great 👍

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@s-rubenstein s-rubenstein merged commit 5c9baf5 into develop Feb 13, 2025
9 checks passed
@s-rubenstein s-rubenstein deleted the sr/dt-1209-abstract-full-view-modal branch February 13, 2025 16:23
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.

3 participants