Fix remaining issues with dashboard and revise layout #551
Conversation
|
This is what it looks like with all the suggestions applied: Images
Text
|
|
Hi @laurasootes Could you have a look at this one and let me know if you agree with the changes I made? The PR turned out a bit bigger than I intended, but I managed to implement all your suggestions in this one. I also did quite a bit of refactoring to reduce some duplication between the different pages. Let me know what you think and if you still find any remaining issues. |
stefsmeets
added this to In progress
in Sprint - 28: Mask generation, finish LIME, setup research experiments for text, Investigate dashboard refactor
via automation
stefsmeets
moved this from In progress
to Ready for review
in Sprint - 28: Mask generation, finish LIME, setup research experiments for text, Investigate dashboard refactor
stefsmeets
changed the title
There was a problem hiding this comment.
Looks good!
Just a few things:
- Can you update the readme with the new dashboard file name?
- In the text tab: is there any way that you could make the label and text align when there is only one line of text?
- SonarCloud seems unhappy
Some suggestions to relax my OCD:
- can you switch the text and image at the home page so that they are in the same order as in the menu?
- Timeseries -> Time series
- home -> Home
|
Thanks for the review @laurasootes , I addressed all your concerns. The sonarcloud warning is because of low coverage for the cli entry point for the dashboard, which is difficult to write a good test for. I hope this adequately satisfies your OCD requirements:
|
This PR fixes some of the issues defined in #549
Resolves #454 - Most of the features of the mockup are present
Resolves #522 - I think the button is in a good place now
Resolves #412 - Dashboard has been refactored to share code between the tabs
Resolves #427 - No longer seems relevant