backstage icon indicating copy to clipboard operation
backstage copied to clipboard

fix: (plugin-tech-radar): Move CSS overflow property to quadrant block element

Open Apaachai opened this issue 3 years ago • 1 comments

Signed-off-by: Vincent Lam [email protected]

Hey, I just made a Pull Request!

Issue link: https://github.com/backstage/backstage/issues/7664

As described in the issue above: when there are too many entries in the tech radar quadrants, some of the sections disappears/renders weirdly. Over time this issue has been somewhat "solved", i.e. the tech radar works great with Google Chrome and Microsoft Edge. However, in the case of other browsers such as Firefox and Safari, the issue still persist:

Screenshot 2022-08-10 at 13 09 21 Image captured in Firefox.
In the quadrant "Process" above, I have added 50+ entries. As we all can see the entries and sections are overflowing into one another.

While debugging this issue I noticed that the tech radar quadrants, i.e. the SVG element behaves/renders differently in different browsers. For some reason the foreignObject is being rendered as an inline-element in Firefox and Safari: Screenshot 2022-08-10 at 16 04 45

while in Chrome and Edge the foreignObject is being rendered as a block-element:

Screenshot 2022-08-10 at 16 04 20

I strongly suspect that this is the reason why the tech radar quadrants is rendered correctly in some browsers and for some other browsers the entries overflows. As the CSS overflow property requires/only works with block elements and that the block element has a defined height.

Hence, this pull request will move the CSS overflow property (in this case: "overflow-y") to a quadrant block element, i.e. in this case to a div element (also known as block-level elements) which is a child element of the <foreignObject> SVG element in the RadarLegend component. By doing so the CSS overflow property will make it possible to render and scroll each tech radar quadrants properly in every browser.

Screenshot 2022-08-10 at 16 02 11 Image captured in Firefox.



My browser version list:

  • Google Chrome Version 104.0.5112.79
  • Microsoft Edge Version 104.0.1293.47
  • Firefox Version 103.0.2
  • Safari Version 14.1.2

:heavy_check_mark: Checklist

  • [x] A changeset describing the change and affected packages. (more info)
  • [ ] Added or updated documentation
  • [ ] Tests for new functionality and regression tests for bug fixes
  • [x] Screenshots attached (for UI changes)
  • [x] All your commits have a Signed-off-by line in the message. (more info)

Apaachai avatar Aug 10 '22 20:08 Apaachai

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-tech-radar plugins/tech-radar patch v0.5.15-next.1

github-actions[bot] avatar Aug 10 '22 20:08 github-actions[bot]

@Apaachai oh dang, I just realized too late after merging - does it work as well for you in all of those browsers if the overflow is set on rings instead of quadrant? Perhaps along with a max-height thrown in for good measure?

The reason I'm asking is, that would potentially be nicer since it'd scroll just all of the entries but leave the heading stationary. Perhaps a bit easier to read if the heading doesn't disappear.

freben avatar Aug 11 '22 07:08 freben

@Apaachai oh dang, I just realized too late after merging - does it work as well for you in all of those browsers if the overflow is set on rings instead of quadrant? Perhaps along with a max-height thrown in for good measure?

The reason I'm asking is, that would potentially be nicer since it'd scroll just all of the entries but leave the heading stationary. Perhaps a bit easier to read if the heading doesn't disappear.

@freben I have tried out your suggestion above like this:

Screenshot 2022-08-11 at 10 30 15

and it also works in all the aforementioned browsers, it looks something like this:

Screenshot 2022-08-11 at 10 31 56

However, as we all might know, whenever we hover over one of the entries, the scrollbar will "reset" to the top. This is due to the fact that the "Radar"/"RadarPlot" component is using useState and onEntryMouseEnter to track/plot each entry (which is causing the re-renders while hovering over the entires). As we can see here:

Screenshot 2022-08-11 at 10 38 12

So if I put the overflow property on the ring or rings, I will be able to scroll for a bit but the scrollbar will more or less reset immediately (as I am scrolling and hovering over the entries at the same time). As it is right know, it is still possible to scroll in between the entry lists (i.e. Adopt, Assess, Hold, and Trial) before selecting one of the entry.

Nevertheless, it's a good suggestion and I agree it would be nice to see the headings while scrolling. Maybe we can move the overflow property to the ring/rings, after it is possible to hover and scroll through the entries in the tech radar quadrants.

Apaachai avatar Aug 11 '22 08:08 Apaachai