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

Use Alert component for track messages #3200

Merged
merged 7 commits into from
Sep 30, 2022
Merged

Use Alert component for track messages #3200

merged 7 commits into from
Sep 30, 2022

Conversation

garrettjstevens
Copy link
Collaborator

I was trying to decide what kind of message might be better than an error in a display as suggested here, and ended up trying to standardize track messages so that they are more predictable for users and therefore hopefully easier to read. This ended up not being strictly related to that PR, so I'm opening it in a separate one.

This uses the Alert component for track messages, whether they are error or info messages. It keeps the ability to override the track message with a custom React component, though.

Here is a before-and-after of these changes:

@garrettjstevens garrettjstevens self-assigned this Sep 20, 2022
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 20, 2022
@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #3200 (563773e) into main (e6c8ca8) will decrease coverage by 0.00%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##             main    #3200      +/-   ##
==========================================
- Coverage   59.38%   59.38%   -0.01%     
==========================================
  Files         672      674       +2     
  Lines       28782    28779       -3     
  Branches     7005     7005              
==========================================
- Hits        17092    17089       -3     
- Misses      11414    11415       +1     
+ Partials      276      275       -1     
Impacted Files Coverage Δ
...w/src/BaseLinearDisplay/models/TooLargeMessage.tsx 90.00% <90.00%> (ø)
...view/src/BaseLinearDisplay/components/BlockMsg.tsx 100.00% <100.00%> (ø)
...play/components/ServerSideRenderedBlockContent.tsx 100.00% <100.00%> (+4.76%) ⬆️
...aseLinearDisplay/models/BaseLinearDisplayModel.tsx 85.86% <100.00%> (-0.10%) ⬇️
plugins/alignments/src/BamAdapter/BamAdapter.ts 72.07% <0.00%> (-0.91%) ⬇️
...ments/src/SNPCoverageAdapter/SNPCoverageAdapter.ts 57.33% <0.00%> (-0.67%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator

pretty nice :) I think this is reasonable and does enhance "first glance readability" a lot

@garrettjstevens garrettjstevens added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Sep 21, 2022
@carolinebridge
Copy link
Contributor

Might be user friendly to add a button to the "zoom in to see sequence" to zoom in one step on the view, could also see if #3158 is addressed / could easily be addressed here

@cmdcolin
Copy link
Collaborator

cmdcolin commented Sep 24, 2022

just for ref #3158 an example screenshot looks like this

Screenshot from 2022-09-23 18-07-41

there are actual "force load" buttons sort of visible oddly enough but it looks a bit off. that said, it is quite hard to correctly design this if trying to fit into the blocks. could be something separate

@garrettjstevens
Copy link
Collaborator Author

Might be user friendly to add a button to the "zoom in to see sequence" to zoom in one step on the view

Added this in a separate PR here: #3221

it is quite hard to correctly design this if trying to fit into the blocks. could be something separate

Agreed

@garrettjstevens garrettjstevens marked this pull request as ready for review September 27, 2022 19:41
@cmdcolin
Copy link
Collaborator

I made a proposed modification to this code. I created a "BlockMsg" component that is used for errors, zoom in to see more message, too much data loaded message, etc. The "BlockMsg" has ellipsis on the text+overflow hidden, and a tooltip to show the full message on mouseover if it is given a very small area. Also removes the Repeater element, as e.g. static blocks are 800px now, and when it was screen width, it was more common to not have any message on the screen at allt. There are still cases the button may be invisible to e.g. force load but that is probably rarer now.

Diff https://github.com/GMOD/jbrowse-components/compare/track_message_updates..track_message_updates_cmd

@cmdcolin
Copy link
Collaborator

example screenshot
Screenshot from 2022-09-30 09-50-36

@cmdcolin
Copy link
Collaborator

(also merged main to this branch just to make diff with my branch easier)

@cmdcolin
Copy link
Collaborator

incidentally, also removed the space between the blocks (can see whitespace between alert elements in https://camo.githubusercontent.com/cee2a7b4669a95efae508a3ce0aba383533f7018cc970c4c1c1afaa44a469118/68747470733a2f2f66696c65732e6769747465722e696d2f3630306233356362643733343038636534666639376664322f68324f532f696d6167652e706e67 but not visible in my screenshots)

I personally like how it looks with no space

@garrettjstevens
Copy link
Collaborator Author

Looks good, I've merged those changes in here

@cmdcolin
Copy link
Collaborator

awesome:)

@cmdcolin cmdcolin merged commit 8b2dfbe into main Sep 30, 2022
@cmdcolin cmdcolin deleted the track_message_updates branch September 30, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants