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 button to breadcrumbs bar to leave feedback #907

Merged
merged 6 commits into from
Jan 10, 2022
Merged

Add button to breadcrumbs bar to leave feedback #907

merged 6 commits into from
Jan 10, 2022

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Dec 14, 2021

I've moved things around a little, hope you like it:

image

I've tried a few other things but wasn't convinced:

  1. image

  2. image

  3. image

@axelboc
Copy link
Contributor Author

axelboc commented Dec 14, 2021

image

Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Not a fan of the new layout. 1. seems more visually appealing to me.

So, the idea would be to add this button for all projects using @h5web/app ?

packages/app/src/breadcrumbs/utils.ts Show resolved Hide resolved
@axelboc
Copy link
Contributor Author

axelboc commented Jan 7, 2022

Not a fan of the new layout. 1. seems more visually appealing to me.

  • What I don't like about layout 1 (below for reference). is that the breadcrumbs are no longer centered within the right-hand side of the UI. They weren't quite centered before, but the Inspect/Visualize control was small enough and the whitespace around the breadcrumbs was big enough that this wasn't noticeable. The feedback button now pushes the breadcrumbs to the left too much in my opinion ... and it pushes them too much to be able to compensate with padding on the left (the whitespace around the breadcrumbs would then feel too unbalanced - i.e. too much whitespace on the left and not enough on the right).

image

  • What I like about the layout I'm proposing (below for reference) is that the alignment of the breadcrumbs is more what we expect it to be on the web (i.e. left aligned); it also equates to less shifting (i.e. the parent crumbs don't move when selecting a child entity). What I also like is that the Inspect/Visualize control is more easily reachable -- we typically want to switch to inspect before/after interacting with the explorer, and at the moment, these two UI elements are on opposite sides of the screen.

image

So, the idea would be to add this button for all projects using @h5web/app ?

With the current implementation, yes. Unless that's not what we want? Do you see a downside?

@loichuder loichuder self-requested a review January 10, 2022 07:41
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

I understand the reasons behind the new layout so we can merge this

@loichuder
Copy link
Member

Unless that's not what we want? Do you see a downside?

I don't have much experience in this so sorry if my question is dumb but won't exposing such a button on https://h5web.panosc.eu/ attract a lot of spam ?

@axelboc axelboc requested a review from t20100 January 10, 2022 07:58
@axelboc
Copy link
Contributor Author

axelboc commented Jan 10, 2022

Ah true, true. Also, it seems @h5web/app is now being used outside the ESRF: #914. 🙌

@t20100
Copy link
Member

t20100 commented Jan 10, 2022

Only downside I can think of with the new layout, is that it adds a bit of visual clutter next to the breadcrumbs.
+1 for left aligned breadcrumbs.

@t20100
Copy link
Member

t20100 commented Jan 10, 2022

BTW, if the "Give feedback" button was smaller, I would have voted for 3. since I'm not sure switching between Inspect and Display is that regular.

@axelboc axelboc force-pushed the feedback branch 2 times, most recently from 3709406 to 89209d9 Compare January 10, 2022 10:58
@axelboc
Copy link
Contributor Author

axelboc commented Jan 10, 2022

As suggested, trying with layout 3 and a shorter label for the feedback button:

image

@axelboc axelboc force-pushed the feedback branch 2 times, most recently from d3dd65c to 4c69001 Compare January 10, 2022 11:45
@axelboc
Copy link
Contributor Author

axelboc commented Jan 10, 2022

/approve

@axelboc axelboc merged commit 5cf8a97 into main Jan 10, 2022
@axelboc axelboc deleted the feedback branch January 10, 2022 14:41
@wlievens
Copy link

wlievens commented Jan 12, 2022

@axelboc If you want some more details on our use case:

I work for a company that designs image sensors. We use the HDF5 format in image sensor validation campaigns, and also plan to use it for volume production test data. A web-based view of HDF5 data would allow users to quickly inspect issues with sensors visually as they get flagged as defective by the production test software. I adapted our (work-in-progress) backend application to comply with h5grove endpoints, and now plan to use h5web in the (Angular) front-end.

This is probably borderline on/off topic for this issue, but I figured since it was about feedback I'd give some :)

@axelboc
Copy link
Contributor Author

axelboc commented Jan 12, 2022

Haha no that's great, thanks a lot for sharing! How did you find the implementation of the H5Grove endpoints? Did you make use of the h5grove library itself? Don't hesitate to open an issue over there if you have any suggestions/questions. 😉

@wlievens
Copy link

The backend uses FastAPI for the REST endpoints, so I made a few GET endpoints there that then call the appropriate functions in h5grove. It wasn't too hard to figure out based on the flaskutils example code.

I already had a (simpler, more limited) HDF5 extraction endpoint for use in our application, so I refactored that client code to re-use the h5grove-compliant hdf5 endpoints. It all works really smooth, except for #914 that is ;-)

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.

None yet

4 participants