Wrappers #109
Wrappers #109
Conversation
Remove configuration and make it available if a tooltip is defined. Add default tooltip for PV widgets.
|
In principle I am happy with these changes, but they would almost certainly have a performance impact so I'm not sure how much re-testing would be necessary. It would be disingenuous to say that we can update this many PVs at this rate when those numbers were generated using a readback widget without tooltip or otherwise. I guess at the least I should investigate the impact before we merge this branch? |
|
With respect to the nested objects, I am happy with unwrapping the wrappers etc but I think keeping containerStyling and widgetStyling separated and enclosed is useful to a widget writer because they can just inject |
|
Disingenuous eh? Surely you were already testing without a tooltip. I think the point is that we need to make sure these performance tests are repeatable without too much grief. It will always be useful to be able to benchmark any changes we make. Re styles: I'm not exactly sure of the pros and cons here but explicit properties are nice because you know exactly what is supported. |
|
Yes the current tests don't use a tooltip, I would imagine that will slow it down a bit, maybe not a lot? It would be quite important that it works really well if we make it the default and I just don't have the numbers right now, but could investigate so we can make a decision before merging? I implemented flat Readback and flat Display along with flat widget and flat PV widget here: 3d7c86b |
|
I don't think it will slow things any more than other changes we are likely to make to the widget components. So making it easy to benchmark is the key I think. |
|
Library for preparing benchmarking is here: https://gitlab.diamond.ac.uk/controls/python3/wdm_performance_test |
I have changed the code such that it behaves in the way I think it did before. In some places this has required defaulting heights and widths to 100%. This is acceptable because some components do not know whether they will be wrapped in another component which will give them size or not. However, I have removed Tooltip from **Widget**, this should be added back in but only when tooltip is required on a **Widget**.
src/components/Widget/widget.tsx
Outdated
| @@ -175,7 +179,7 @@ export const Widget = (props: WidgetComponent): JSX.Element => { | |||
| let { baseWidget, widgetStyling = {}, ...baseWidgetProps } = containerProps; | |||
|
|
|||
| // Put appropriate components on the list of components to be wrapped | |||
| let components = [baseWidget]; | |||
| let components = [TooltipWrapper, baseWidget]; | |||
There was a problem hiding this comment.
Default is currently to have a tooltip for a Widget, not just PVWidget
Should we change this? Widget could add tooltip if props.tooltip is not undefined?
| style={{ | ||
| position: "relative", | ||
| height: "100%", | ||
| width: "100%", | ||
| ...style | ||
| }} | ||
| > |
There was a problem hiding this comment.
Something to do with the styling of this component is not working as it should, see the flexible page for an example of how it is stretching things beyond their specified sizes. Am investigating now.
Performance TestingChecked render times against the readback with no wrappers and found only small differences:
This does not concern me that much |
|
However, when rendering with the tooltip open, this appears:
This is trying to tell us that we are triggering more updates within an update, and is generally a waste and a bad thing. See these issues:
This is worthy of investigation as it adds significant overhead to the render time when the tooltip is being displayed - one readback takes around 16ms when this is happening. This appears to be an approximately constant overhead, adding more widgets does not increase the time spent in these cascaded changes. Nevertheless it is indicative of the fact that we could write this part of the code better. |
|
This issue arises from the componentDidUpdate lifetime hook in the Popover package I've looked at it a bit and I have tracked it down to the call to @willrogers are there any similar popover packages we could try out which might avoid this issue? Or can we live with it for the time being? I could create an issue but not sure how actively it is being worked on. |
Had to add a default 100% 100% sizing to the Display component, this seems to be fine for the time being although these default sizes were not necessary before?
Restored styling capabilities
The menuWrapper will now appear if the widget has any actions, and any widget may now have actions.
|
Some styling issues persist, perhaps changes similar to similar to #113 are required? I will investigate this with a similar approach. Also need to provide a default description for actions so that context menu on right-click does not appear blank to user. |
|
Good job on flattening the JSON though, this will work nicely with #114 |
Not a problem now but would crop up in the future if menuwrapper were to be wrapped with something else
Any widgets wrapped inside a container will gain height and width of 100% and pop to the correct size. This actually works so well that I have been able to remove a couple of default stylings, for instance from alarmBorder.module.css and the Progress page actually looks better :)
Not a problem now but would crop up in the future if menuwrapper were to be wrapped with something else
Any widgets wrapped inside a container will gain height and width of 100% and pop to the correct size. This actually works so well that I have been able to remove a couple of default stylings, for instance from alarmBorder.module.css and the Progress page actually looks better :)
…b-proto into wrapper-styling-fixes
Wrapper styling fixes
@TimGuiteDiamond a little rearrangement.
TooltipWrapper is now always added if a
tooltipproperty is there.I plan to remove the
wrappersnested object, always add menu button and make alarm border a top-level boolean.