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

Move Subject Table Validation Inside + Properly Copy-Paste Sessions (when empty) #491

Merged
merged 12 commits into from
Nov 2, 2023

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Oct 31, 2023

This PR moves all of the subject table validation steps inside the Table itself—requiring that Subject ID and Sessions are both provided to validate.

A fix for session copy-pasting is also provided, though this only works when the Subject cell is empty (because of an internal HOT issue #153)

@garrettmflynn garrettmflynn self-assigned this Oct 31, 2023
@garrettmflynn garrettmflynn changed the title Move Subject Table Validation Inside + Allow Copy-Pasting Sessions (when empty) Move Subject Table Validation Inside + Properly Copy-Paste Sessions (when empty) Oct 31, 2023
@garrettmflynn garrettmflynn changed the base branch from main to global-metadata-on-demand October 31, 2023 23:53
Base automatically changed from global-metadata-on-demand to main November 1, 2023 15:00
@CodyCBakerPhD
Copy link
Collaborator

As discussed, attempt to fix the issue where I add a new subject but use an existing ID, have it not override all the previous work

Perhaps automatically append some index/format to the new duplicated ID, similar to what happens when you copy+paste a file in the same directory on most OS?

@garrettmflynn
Copy link
Member Author

Fixed! I'm hesitant to add anything to the ID automatically, as this'll likely only happen for naive copy/paste operations—where I'd rather let the user know they need to input their own unique value.

Additionally, this'll be managed internally for the next iteration of this (i.e. two tables). So will go away as a possible problem soon.

@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn Small conflict now

@garrettmflynn
Copy link
Member Author

Fixed!

@CodyCBakerPhD
Copy link
Collaborator

OK sure, an example like this is self evident

image

@CodyCBakerPhD
Copy link
Collaborator

CodyCBakerPhD commented Nov 2, 2023

@garrettmflynn One question arose w.r.t. button placement in Chromatic (may have occurred in previous PR without me realizing until now)

If unrelated to changes of this PR we can go ahead and merge as-is and follow up with a specific fix for that (or if intentional, let me know and we can discuss further)

@garrettmflynn
Copy link
Member Author

You're right, this is a mistake that was on main. I've pushed there directly—so it should be gone.

@CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn Cool, just a small conflict here now

@CodyCBakerPhD CodyCBakerPhD merged commit 35d005a into main Nov 2, 2023
7 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the selectively-validate-empty-values branch November 2, 2023 20:12
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