-
Notifications
You must be signed in to change notification settings - Fork 0
Add beamline stats fields and start tidying up #53
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
Conversation
|
while reviewing this I noticed that a recent change to blueapi has broken the dev configuration, so I raised DiamondLightSource/mx-bluesky#1168 This fix seems to have worked for me: |
src/screens/BeamlineStats.tsx
Outdated
| display: "flex", | ||
| }} | ||
| > | ||
| <Stack alignItems={"center"}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I staggered the columns on purpose actually - don't really like it but it made things more readable across different screen sizes than when they lined up (still figuring out css details so... workaround)
Making them line up is fairly straightforward - can try again, but need to figure out the resizing in css
Ah, yeah, that was known (lots of breaking changes in newer blueapi versions - the one on the beamline is fairly old now) but hadn't got around to writing the ticket yet so thanks! :) There a PR now: DiamondLightSource/mx-bluesky#1171 |
| } | ||
| > | ||
| Move to Pilatus! | ||
| </Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've had this conversation before about the button highlight colour being almost invisible in both light and dark modes. Is there an issue logged about this in the upstream widget/theming that we use? Either that or perhaps we are somehow using them wrong - I'm not an MUI expert.
Honestly I think it's pretty piss poor usability that the buttons don't provide any real affordance that they are clickable unless you roll over them. They should look like buttons!
If you look at the documentation, for MUI https://mui.com/material-ui/react-button/ and also for the original material design, and how the buttons are used in apps everywhere, text buttons are generally rendered in light blue, either blue-on-white or white-on-blue
There is a reason for this, it's because it tells you that it's a button and you can click it! Also, the button text is in all caps. I'm increasingly annoyed about this and I think it should be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buttons in the docs are like that because that's the default theme - which I agree looks much better. Just made a commit making the bunntons "contained" meaning they are more visible (in you words: they look like buttons) - at least in dark mode. I can't seem to get the light mode however to do the same - which may be limited by the known issues in the theme we're using...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I can also make a custom version of the button - which I think is overkill considering we already have the MUI one but we're limited by the color scheme of the theme - but that's a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should raise a bug against the diamond theme then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They already have a PR that fixes the theme - apparently the underlying issue was subclassing the theme not working properly - I'm just waiting for it to get merged so we can start using the new one and hopefully that will improve matters. For now the only other thing I can do is make all the buttons contained and default to secondary color (which is yellow) which will make them equally visible in both light and dark mode
edit: just noticed that particular PR got merged, just waiting for release then!
| import { WorkerStatus } from "../components/WorkerStatus"; | ||
|
|
||
| type StateBoxProps = { bgColor: string; title: string; titleColor: string }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this page and the detector position page generally needs some additional structure like some cards to group together the information and make it more organised, there's a lot of white space and I prefer the other tabs where everything is a bit more organised.
Also the centring of the columns means that when the information changes, the text jumps around which is distracting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The aim is to eventually have all the information in this page end up in a permanent drawer (see #46 ) as the scientists usually prefer having this information always visible during a collection for debugging purposes... that might take awhile to achieve as it involves rethinking most of the layout for other pages as well.
For now, cards sound like a good idea - will try that
rtuck99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much nicer. Approved

Closes #8
Closes #23
This also starts tidying up the coustom PV components and types - for now just to be able to reuse them. Work on this will be continued in a separate PR (issue incoming)