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

Add an Invocations Panel for the Invocations activity #18137

Merged
merged 3 commits into from
May 20, 2024

Conversation

ahmedhamidawan
Copy link
Member

Requires #18088

invocations_activity_panel_third.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.

i.e.: Do not render the `<- Invocations List` button if you open the invocation state overview from the invocations panel
Maybe we should consolidate styling for all scroll lists
@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review May 20, 2024 13:55
@dannon dannon merged commit 9843034 into galaxyproject:dev May 20, 2024
30 of 31 checks passed
@mvdbeek
Copy link
Member

mvdbeek commented May 20, 2024

This is a step backwards in usability, can we revert that for the time being ?

@mvdbeek
Copy link
Member

mvdbeek commented May 20, 2024

I guess it'd have been nice to have a little bit of time to review this, but maybe it is worth getting this in front of users. Could we get the full invocation list button at the top of the panel, and is it possible to keep the invocation activity we have on 24.0 as an option that people like me could continue to use ?

@dannon
Copy link
Member

dannon commented May 20, 2024

@mvdbeek We talked about this at the UI/UX meeting last week, it's not like this was rushed in -- at the time you mentioned you were concerned about the amount of real estate each invocation has in the panel, but that it'd be good to get in front of more people for more opinions. I agree.

@mvdbeek
Copy link
Member

mvdbeek commented May 20, 2024

Screenshot 2024-05-20 at 17 39 23

@mvdbeek
Copy link
Member

mvdbeek commented May 20, 2024

(to be fair the 1 hour is likely more than that, with rounding)

@dannon
Copy link
Member

dannon commented May 20, 2024

Thank you for the screenshot. This PR has been open over a week, and the timestamps you're showing there are showing the simple rebase on top of the parent. I don't know what point you are making.

@mvdbeek
Copy link
Member

mvdbeek commented May 20, 2024

That it was a draft until an hour ago. There's so much going on that I don't have time to review every draft, but given I had expressed concerns earlier it would have been nice (but not a must) to have some time to review this. It's my fault of course, I could've requested changes earlier on.

@dannon
Copy link
Member

dannon commented May 20, 2024

Sure, a draft so we could discuss it like we did during the call last week, and in draft waiting on the parent PR to be merged so it could be rebased. Again, this was not rushed, there's been a week post-discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants