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

Switch events without reload #37 + auto event select #34 #38

Merged
merged 11 commits into from
Jun 28, 2024

Conversation

brauliorivas
Copy link
Member

BEGINRELEASENOTES

ENDRELEASENOTES

@brauliorivas brauliorivas linked an issue Jun 19, 2024 that may be closed by this pull request
@brauliorivas brauliorivas linked an issue Jun 19, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Jun 19, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-06-28 14:03 UTC

@tmadlener
Copy link
Contributor

Very nice.

I observed the following issue: If I switch events very quickly in the tree view, I can trigger a mis-render, where I think what is happening is that the symbols from one event are overlaid onto another event. Or maybe the symbols of the current event are drawn twice but once out of position (see screenshot on the left)
image

And then I would also have an additional feature request (which might already solve the first one). Instead of having the lef-right arrows for the event selection in the tree view, I think I would also like a drop-down menu there. Or if both options are possible simultaneously, e.g. by having the arrows for switching, and if I click on the event itself I get a dropdown menu to select.

@brauliorivas
Copy link
Member Author

And then I would also have an additional feature request (which might already solve the first one). Instead of having the lef-right arrows for the event selection in the tree view, I think I would also like a drop-down menu there. Or if both options are possible simultaneously, e.g. by having the arrows for switching, and if I click on the event itself I get a dropdown menu to select.

I didn't specify it, but indeed there is a dropdown menu. If you click over the event number, then you can see the available events and choose between them (so both options are present, arrows + menu).
demo-event-switch

@tmadlener
Copy link
Contributor

Huh, I didn't see that (or didn't figure it out at least). Very nice then. I am wondering whether it could be made a bit more obvious that this is clickable and will give you a list to select from.

@kjvbrt
Copy link
Collaborator

kjvbrt commented Jun 21, 2024

This is nice :)

  • Can you unify the font sizes in the "menus"? It looks to me that the font size of the event switcher is a bit larger
  • Would it be possible to keep window in the position from the previous event?
  • What happens with the dropdown if there is many events to select from? Is there any scrollbar?

@tmadlener
Copy link
Contributor

More comments from the meeting:

  • Event selection in the tree view should have a max height and a scroll bar for event selection
  • Drawing of symbols is lagging a bit, so that symbols from previous events are visible. We could document that toggling the PDG IDs, can fix things
  • Maybe delay start drawing of events after the selection to avoid some issues when quickly switching events

@brauliorivas
Copy link
Member Author

brauliorivas commented Jun 22, 2024

be made a bit more obvious that this is clickable and will give you a list to select from.

  • Now, when users hover on an element, the background color changes + underline.

Can you unify the font sizes in the "menus"? It looks to me that the font size of the event switcher is a bit larger

  • Oh didn't notice this 😄, fixed.

Would it be possible to keep window in the position from the previous event?

  • Done!

What happens with the dropdown if there is many events to select from? Is there any scrollbar?

  • The scrollbar is now implemented.

Maybe delay start drawing of events after the selection to avoid some issues when quickly switching events

  • Hmm, this is a technical issue and somewhat "hard" to fix. Even if we add a setTimeOut function, and disable the arrows to switch between events, the code that draws the PDGs has an event on it, that forces to draw sooner or later the image for MCParticles. We could make this code async, so we always wait for the image to load even if is already loaded but then we have to make all drawing-related code async, and this could affect other functions behave. For example, filtering or getting visible particles. This causes a bit of noise when drawing. So taking into account the trade-offs, we may want to leave it as it is. The simplest way to show the user a "loading" state, is to change the event number after the objects have been drawn (we can not take for granted that all images will be loaded). I would like to know what you both @tmadlener and @kjvbrt think, we can try the option I mentioned. But this is how I see this.

@tmadlener
Copy link
Contributor

Very nice work on the visuals. I like it.

If we cannot change the drawing easily, can we put a loading ... or drawing ... on the screen, or maybe simply make the arrows / event selection unclickable until it's done? Otherwise we keep it as it is at the moment and open an issue to keep track of it.

@brauliorivas
Copy link
Member Author

brauliorivas commented Jun 24, 2024

Otherwise we keep it as it is at the moment and open an issue to keep track of it.

Yeah, I would prefer to open an issue and not lose this. I know how to fix it, but I would have to incorporate a lot of "boilerplate" async code. However, drawing functions may change (making these changes in vain), so right I wouldn't worry about it too much.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this as it is. Can you open an issue for keeping track of the async drawing before merging this? Unless @kjvbrt still wants changes.

@kjvbrt
Copy link
Collaborator

kjvbrt commented Jun 25, 2024

Would it be possible to keep window in the position from the previous event?

* [ ]  Done!

The position is still reset for me (to x: 0, y: middle). I'm on latest Firefox in Fedora 40.

@kjvbrt
Copy link
Collaborator

kjvbrt commented Jun 25, 2024

Can you also harmonize shades of grey in the "menus"? I found three different ones :)

image

@kjvbrt kjvbrt mentioned this pull request Jun 25, 2024
@brauliorivas
Copy link
Member Author

The position is still reset for me (to x: 0, y: middle). I'm on latest Firefox in Fedora 40.

Could you please try "hard" reloading the website (maybe ctrl + r)? I think it's cached because it works for me both in the preview and on firefox 126.0.1.

@brauliorivas
Copy link
Member Author

Can you also harmonize shades of grey in the "menus"? I found three different ones :)

Of course!
image

@kjvbrt
Copy link
Collaborator

kjvbrt commented Jun 26, 2024

The position is still reset for me (to x: 0, y: middle). I'm on latest Firefox in Fedora 40.

Could you please try "hard" reloading the website (maybe ctrl + r)? I think it's cached because it works for me both in the preview and on firefox 126.0.1.

Still does not work, but let's merge this PR and see. If it does not work I will create issue...

@kjvbrt kjvbrt merged commit 0ad8a09 into key4hep:main Jun 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants