webknossos icon indicating copy to clipboard operation
webknossos copied to clipboard

Folder And Dataset Properties

Open MichaelBuessemeyer opened this issue 1 year ago • 1 comments

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • abc

TODOs:

  • [x] Clean up git history
  • [ ] Allow to search for metadata props -> Other issue
  • [x] Add props & UI and everything for folders
  • [x] Update UI to the newest design proposal
  • [ ] Clearn up code

Issues:


(Please delete unneeded items, merge only when none are left open)

  • [ ] Updated changelog
  • [ ] Updated migration guide if applicable
  • [ ] Updated documentation if applicable
  • [ ] Adapted wk-libs python client if relevant API parts change
  • [ ] Removed dev-only changes like prints and application.conf edits
  • [ ] Considered common edge cases
  • [ ] Needs datastore update after deployment

MichaelBuessemeyer avatar Jun 17 '24 15:06 MichaelBuessemeyer

For the record:

During development / testing, I noticed an error with the antd Select component used in the new MetadataTable. Looks like this is this one of the related issues: https://github.com/ant-design/ant-design/issues/46348. I just gave it a quick look. The error might be gone after a proper antd & react update :)

image

MichaelBuessemeyer avatar Jul 05 '24 11:07 MichaelBuessemeyer

Things I noticed during testing:

  • [x] The type a,b always has an empty tag as a default Screenshot 2024-07-17 at 11 23 30

  • [ ] No matter which dataset I selected from my dashboard they all had the same values Screenshot 2024-07-17 at 11 27 29

Some UI/UX feedback:

  • [ ] The colon between "property" and "value" is not centered. Better yet, we can remove it.
  • [x] The properties are always displayed as text boxes even when one does not want to edit them. Text boxes are not great for readability. Maybe make the inputs borderless when they are not actively being edited (https://ant.design/components/input#components-input-demo-variant)
  • [ ] Similar to the comment above, there is very little space in general. Maybe the "type" dropdown" and the "delete" button don't always have to be there. They are only relevant when working/editing an entry. Similar to other UI elements in WK every entry can enter an edit mode where all of that stuff is displayed. When not in editing mode, then the properties are just displayed as text. To avoid a pencil icon, one could trigger the edit mode when clicking on the text similiar to the antd <Typography.Text> component (https://ant.design/components/typography#components-typography-demo-editable).
  • [ ] Have a look a the antd <Space.Compact> component for clean compact input fields (https://ant.design/components/space#space-demo-compact). e.g. Screenshot 2024-07-17 at 11 57 18
  • [ ] The "types" field is only relevant for the values. All keys are always strings anyway. Therefore the "type" dropdown should can be right next to the "value" input and need not be the first entry in the table. (see comment directly above)
  • [ ] I am not a big fan of the "plus" button to add new entry. Especially the empty state looks weird with just the table header and button. Maybe always show and empty to row, ready to edit?
  • [ ] I wonder if the "types" options could be better explained through icons. Especially the multiple values/tags thing is not obvious. Antd has several options that might make sense, e.g. <TagsOutlined /> Screenshot 2024-07-17 at 11 38 27 Screenshot 2024-07-17 at 11 40 52

hotzenklotz avatar Jul 17 '24 10:07 hotzenklotz

The type a,b always has an empty tag as a default

Thanks, I remove fix that

No matter which dataset I selected from my dashboard they all had the same values

Afaik, these are default added values for a new dataset. I can change that in the backend. You can change them individually for each dataset and the should be shown and saved individually.

The colon between "property" and "value" is not centered. Better yet, we can remove it.

The colon was explicitly requested to be added. I centered it now.

The properties are always displayed as text boxes even when one does not want to edit them. Text boxes are not great for readability. Maybe make the inputs borderless when they are not actively being edited (https://ant.design/components/input#components-input-demo-variant)

The inputs were already borderless. But I now also made the background transparent when the entry is not being edited.

Similar to the comment above, there is very little space in general. Maybe the "type" dropdown" and the "delete" button don't always have to be there. They are only relevant when working/editing an entry. Similar to other UI elements in WK every entry can enter an edit mode where all of that stuff is displayed. When not in editing mode, then the properties are just displayed as text. To avoid a pencil icon, one could trigger the edit mode when clicking on the text similiar to the antd <Typography.Text> component (https://ant.design/components/typography#components-typography-demo-editable). Have a look a the antd <Space.Compact> component for clean compact input fields (https://ant.design/components/space#space-demo-compact). e.g.

Thanks something to be discussed (I'd say as I got a different design proposal).

The "types" field is only relevant for the values. All keys are always strings anyway. Therefore the "type" dropdown should can be right next to the "value" input and need not be the first entry in the table. (see comment directly above)

The whole type dropdown was removed to save some space in the table. Users can now select a type when creating a new entry and that type will no longer be changable after creation.

I am not a big fan of the "plus" button to add new entry. Especially the empty state looks weird with just the table header and button. Maybe always show and empty to row, ready to edit?

Initially there always was an empty row. But it was decided against this. See https://scm.slack.com/archives/C5AKLAV0B/p1719923640762759 and https://scm.slack.com/archives/C5AKLAV0B/p1718896223081789

I wonder if the "types" options could be better explained through icons. Especially the multiple values/tags thing is not obvious. Antd has several options that might make sense, e.g. <TagsOutlined />

Thanks, I did know about these icons. I added them to the newest version.

MichaelBuessemeyer avatar Jul 17 '24 14:07 MichaelBuessemeyer

My guess is that the publications rendering code from the about page of webknossos needs to be adapted. Do you think that would be straightforward?

Yes, I can take of this once this PR is deployed.

normanrz avatar Jul 22 '24 11:07 normanrz

@philippotto I prepared the code ready for your feedback and testing :D.

Moreover, I just removed an unnecessary dependency of a useEffect hook introduced by me in another pr which was just merged today. See: https://github.com/scalableminds/webknossos/pull/7935#discussion_r1695055434

MichaelBuessemeyer avatar Jul 29 '24 11:07 MichaelBuessemeyer

@markbader Metadata should now be updateable via the normal dataset update route

MichaelBuessemeyer avatar Aug 07 '24 10:08 MichaelBuessemeyer

Before merging the migration & revision needs to be tested. I already tested this for me locally by:

  • check out master
  • run yarn refresh-schema
  • open wk dashboard
  • Add some tags for at least one dataset and remember them
  • Open the datasets table of the Postgres db manually and add some "species", "acquisition", and "brainRegion" to the details like done for publications
  • Then checkout this branch
  • Run the migration on the db -> this should succeed
  • open the wk dataset dashboard and take a look at the metadata of the datasets that previously had tags. They should now be transferred to a tags entry
  • use the dbtool to run a schema diff. This should run successfully without showing any schema diffs
  • run the revision SQL script
  • checkout master
  • run schema diff -> should also show no schema diff
  • Check the dataset dashboard: The previously added tags should appear there

MichaelBuessemeyer avatar Aug 16 '24 07:08 MichaelBuessemeyer

the front-end looks good to me :+1: leaving final approval to @fm3. he probably should have a look at the migration code, too. also, does the python lib need to be adapted to this pr?

philippotto avatar Aug 19 '24 13:08 philippotto

he probably should have a look at the migration code, too.

Yeah definitely.

Moreover: as the tags should be deprecated should they be remove from the schema and API objects returned by the API or should they stay but the frontend cannot update and view them?

Keeping them, doesn't require us to bump the API (I would say) but would also increase our technical dept?!

MichaelBuessemeyer avatar Aug 19 '24 13:08 MichaelBuessemeyer

Ok what to do with tags :)

I just talked with @normanrz and the conclusion is to keep tags as some are using them as well as the pretty important filtering feature.

If we later want to merge the tags into the metadata, we can still do that. In case dataset/folder already has a metadata field named "tags" already exists, we just come up with something :).

MichaelBuessemeyer avatar Aug 21 '24 12:08 MichaelBuessemeyer

testing went well. Tags should be restored in the frontend :)

MichaelBuessemeyer avatar Aug 22 '24 08:08 MichaelBuessemeyer

And @fm3 no need to test the migration again. I tested it and it should work :tm: :see_no_evil:

MichaelBuessemeyer avatar Aug 27 '24 13:08 MichaelBuessemeyer