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

Meeting Updates from 1/29/2024 #578

Merged
merged 13 commits into from
Jan 30, 2024
Merged

Meeting Updates from 1/29/2024 #578

merged 13 commits into from
Jan 30, 2024

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Jan 30, 2024

This PR implements the cosmetic updates—as well as a few related bugs—from yesterday's meeting, including:

  1. Make table borders darker (to see when both are red)
  2. Remove n fields counter
  3. Make the accordions less obtrusive (remove padding)
  4. Ensure all top-level metadata is required (so no exclusion is possible)
  5. Remove control + control_description + comments everywhere
  6. Replace checkbox with “Skip” button
  7. Fix validation using Species.sex on the Subject Table as a test case
  8. Ensure you can submit global metadata with loose requirements (i.e. not validated when empty)
Screenshot 2024-01-30 at 10 56 05 AM Screenshot 2024-01-30 at 10 58 02 AM

Remaining issues that can be tackled in a follow-up include:

  1. Provide a better description for what resolution means
  2. Ensure that DfOverF—as well as the other nested Ophys properties—cannot be skipped (required, if present, on the NeuroConv side)
  3. Provide a test case for multiple imaging planes

@garrettmflynn garrettmflynn self-assigned this Jan 30, 2024
@CodyCBakerPhD
Copy link
Collaborator

Make the accordions less obtrusive (remove padding)

Still looks like it could go down by another increment on this

@CodyCBakerPhD
Copy link
Collaborator

Also tests failing; does that represent a back-compatibility break?

@garrettmflynn
Copy link
Member Author

represent

Since the proper validation has been restored, the tests were failing because ElectrodeColumns was not specified on the test data but now we're correctly trying to check this to see if all our columns are valid.

I've just ignored the check if ElectrodeColumns is blank.

@garrettmflynn
Copy link
Member Author

Good here? Turns out I'd been holding it at a min-height value which kept some "padding" despite removing any explicit declaration.
Screenshot 2024-01-30 at 11 19 08 AM

@garrettmflynn
Copy link
Member Author

The height is still able to grow to fit auxiliary elements like the Skip button.
Screenshot 2024-01-30 at 11 20 15 AM

@CodyCBakerPhD
Copy link
Collaborator

Good here? Turns out I'd been holding it at a min-height value which kept some "padding" despite removing any explicit declaration.

Better, maybe one more increment?

Also is it possible to make that box a bit darker gray than the background?

Also note that for DfOverF/Fluorescence/SegmentationImages they weren't supposed to be 'skippable'; that change was only for the source data selector

@garrettmflynn
Copy link
Member Author

Better, maybe one more increment?
I can remove the padding entirely, but this will make things difficult to read and click. The previous image was at 5px at the top and bottom.

Here is 0px.
Screenshot 2024-01-30 at 11 25 18 AM

This is 3px.
Screenshot 2024-01-30 at 11 26 20 AM

Also note that for DfOverF/Fluorescence/SegmentationImages they weren't supposed to be 'skippable'; that change was only for the source data selector
I thought we'd agreed that this is best dealt with through updates to the way that NeuroConv handles the schema. I can patch over this on the GUIDE end, but I didn't want to forget that we have some issues with the schema at a lower level.

@CodyCBakerPhD
Copy link
Collaborator

I thought we'd agreed that this is best dealt with through updates to the way that NeuroConv handles the schema. I can patch over this on the GUIDE end, but I didn't want to forget that we have some issues with the schema at a lower level.

What specific issue on the NeuroConv side? If the NeuroConv schema request returns it we can proceed to assume it's required for the GUIDE

@garrettmflynn
Copy link
Member Author

Is this dark enough? Happy to take an rgb value from you, this is rgb(235, 235, 235)
Screenshot 2024-01-30 at 11 28 12 AM

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Jan 30, 2024

What specific issue on the NeuroConv side? If the NeuroConv schema request returns it we can proceed to assume it's required for the GUIDE

As Ben mentioned yesterday, if there's no way to not pass DfOverF or any of the other properties (e.g. Subject, Ophys, etc., both of which I've patched because they're signficantly more noticeable) to NeuroConv, the schema should include it in the required list.

This would require dynamically changing the returned schema, which may present a more substantial change that we can readily implement now.

@garrettmflynn
Copy link
Member Author

Alright just pushed an update to require all sub-levels of the Ophys metadata, if included in the schema.

@garrettmflynn
Copy link
Member Author

garrettmflynn commented Jan 30, 2024

Also once the accordion is opened, it doesn't matter how small the padding was because the content pushes all the others beneath the fold.

IMO the size of these dropdown headers now aren't well-distinguished from the content, making it all more difficult to process once open. Though these screenshots don't really do this justice.

Screenshot 2024-01-30 at 11 41 53 AM

@CodyCBakerPhD
Copy link
Collaborator

Is this dark enough? Happy to take an rgb value from you, this is rgb(235, 235, 235)

This is pretty bizarre... Maybe it's happening with your screenshots too, but when I take a screenshot of what it looks like to me the gray gets whitened

image

(the one that looks gray there is from me hovering over it, which was even a shade darker than the default grayness)

On my screen, the current gray looks OK

3px on the padding looks good

Alright just pushed an update to require all sub-levels of the Ophys metadata, if included in the schema.

Thanks; it will take quite some thought about how to do something similar on the NeuroConv side since the way you interact with metadata is starting to become very very different. For example, if the key reference to a plane changes in the metadata instance (which is perfectly allowed in the programmatic side) you'd also have to update the requirements to exactly match that update to the pattern property in the schema I think. Which would be really weird

In the end I think they are becoming two separate things: the GUIDE is a very limited scoped way of altering metadata fields on a fixed instance of the metadata dict, constrained by the default schema for each interface. So I think saying the required = properties here specific to the GUIDE is perfectly OK

@CodyCBakerPhD
Copy link
Collaborator

IMO the size of these dropdown headers now aren't well-distinguished from the content, making it all more difficult to process once open.

Fixable by making the font size of the headers slightly larger, no?

@garrettmflynn
Copy link
Member Author

This is pretty bizarre... Maybe it's happening with your screenshots too, but when I take a screenshot of what it looks like to me the gray gets whitened

Hmmm weird. I used a color picker on your image and it's still the original rgb(240,240,240). Must be a perceptual illusion as a result of all the other grays in the application.

@garrettmflynn
Copy link
Member Author

Fixable by making the font size of the headers slightly larger, no?

Actually, it looks best to me if I reset the location of the colored status bar to the bottom (rather than the left). Changing the font size will start messing with the visual balance, requiring more padding to look intentionally designed.

Screenshot 2024-01-30 at 11 57 57 AM

@CodyCBakerPhD
Copy link
Collaborator

Actually, it looks best to me if I reset the location of the colored status bar to the bottom (rather than the left). Changing the font size will start messing with the visual balance, requiring more padding to look intentionally designed.

Oky doky

Just keep in mind I'm still of the opinion that top level tabs (like the way the pydata docs theme looks) instead of accordions would remedy all of these issues

@garrettmflynn
Copy link
Member Author

Right. I'll add that back to my to-dos and see if I can get that implemented shortly.

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) January 30, 2024 21:01
@CodyCBakerPhD
Copy link
Collaborator

Right. I'll add that back to my to-dos and see if I can get that implemented shortly.

Sounds good, no rush if it's tricky. Was just a dream

The rest of these cosmetic updates look/feel good

@CodyCBakerPhD CodyCBakerPhD self-requested a review January 30, 2024 21:49
@CodyCBakerPhD CodyCBakerPhD merged commit 2eb2d7a into main Jan 30, 2024
11 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the cosmetic-updates branch January 30, 2024 21:55
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