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

Visually indicate currently viewed/edited dataset #16859

Merged

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Oct 16, 2023

Fixes #16784

Indicates which dataset is currently being viewed/edited in ContentItem using a left-border:

content_item_current_indicator_4.mp4
content_item_current_indicator_3.mp4

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@mvdbeek
Copy link
Member

mvdbeek commented Oct 20, 2023

This is very cool, but I wonder if there is something more standard than the white color ? Could it be an outline change or some color variant or something along those lines ?

@dannon
Copy link
Member

dannon commented Oct 20, 2023

I should have updated here; @ahmedhamidawan and I discussed this out of band and the plan is to shift back to the sticker whole-card-highlight, instead of the white per-interface icon.

@dannon dannon marked this pull request as draft October 20, 2023 14:50
@ahmedhamidawan ahmedhamidawan force-pushed the content_options_active_indicator branch from 30234d8 to e62fb3b Compare October 23, 2023 17:35
@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review October 23, 2023 22:21
@dannon dannon force-pushed the content_options_active_indicator branch from 5c6a379 to 7eb965e Compare October 25, 2023 14:25
@ahmedhamidawan ahmedhamidawan marked this pull request as draft October 26, 2023 01:42
@ahmedhamidawan ahmedhamidawan force-pushed the content_options_active_indicator branch from 209b117 to 9a14a5f Compare October 26, 2023 15:28
@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review November 7, 2023 22:29
@ahmedhamidawan
Copy link
Member Author

Failing tests:

FAILED lib/galaxy_test/selenium/test_workflow_editor.py::TestWorkflowEditor::test_edit_license - AssertionError: Expected data-license to have value [MIT] but had value [null]

FAILED lib/galaxy_test/selenium/test_histories_published.py::TestPublishedHistories::test_published_histories_sort_by_name - AssertionError: assert '9viqk239u1' == '143d99z7rw'

seem unrelated to the ContentItem changes made here

@martenson
Copy link
Member

martenson commented Nov 17, 2023

Overall I like the style and I think it will help people navigate histories.

Couple of cases that might use some polish:

  • here I clicked (eye) icon on #12 and then expanded #10 -- it is a bit confusing since two things are "highlighted" at the same time
    Screenshot 2023-11-17 at 9 56 08 AM
  • the ribbon shows for eye, edit, bugreport, info buttons but does not show for rerun, visualize, help buttons -- this feels arbitrary, why some but not the others?

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Nov 18, 2023

Thank you @martenson !

here I clicked (eye) icon on #12 and then expanded #10 -- it is a bit confusing since two things are "highlighted" at the same time

The ribbon indicates the dataset currently being "worked on" (or viewed) in the center panel, so i feel it makes sense to keep that highlighted, no? I wouldn't think you would want it to go away if you click another item, because expanding the other item still might not mean the first one isn't being operated on in the center

the ribbon shows for eye, edit, bugreport, info buttons but does not show for rerun, visualize, help buttons -- this feels arbitrary, why some but not the others?

The ribbon only shows for those views since they are directly related to the dataset (also because the id is in the URL), whereas, rerun is emitted for the current dataset but one can change the chosen dataset value in the Tool form, at which point the ribbon wouldn't make sense

@martenson
Copy link
Member

Please see responses below. Overall I suggest let's roll with this and see if we get more feedback.

because expanding the other item still might not mean the first one isn't being operated on in the center

My read would be that you are more likely to have shifted context to the newly selected than not.

The ribbon only shows for those views since they are directly related to the dataset (also because the id is in the URL), whereas, rerun is emitted for the current dataset but one can change the chosen dataset value in the Tool form, at which point the ribbon wouldn't make sense

Visualization is directly related to the dataset. I give you that the other two I mentioned are less related though.

@martenson martenson merged commit 4cad7e7 into galaxyproject:dev Nov 28, 2023
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[Feature request] Add a marker to currently viewed dataset
4 participants