Conversation
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
|
|
||
| export const CustomPlotsWrapper: React.FC = () => { | ||
| const { plotsIds, nbItemsPerRow, isCollapsed } = useSelector( | ||
| const { plotsIds, nbItemsPerRow, isCollapsed, height } = useSelector( |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| <> | ||
| <div | ||
| className={styles.sizeSliders} | ||
| data-testid="nb-items-per-row-slider" |
There was a problem hiding this comment.
Should we rename this id since it now holds two different sliders? Or maybe give each slider their own id 🤔
| /> | ||
| </div> | ||
| {changeSize && hasItems && maxNbPlotsPerRow > 1 && ( | ||
| <> |
There was a problem hiding this comment.
Is this <></> wrapper needed?
| } | ||
|
|
||
| .ratioVerticalNormal .plot { | ||
| aspect-ratio: 3 /4; |
There was a problem hiding this comment.
I'm assuming we want to keep things consistent 😄
| aspect-ratio: 3 /4; | |
| aspect-ratio: 3 / 4; |
| className={styles.sizeSliders} | ||
| data-testid="nb-items-per-row-slider" | ||
| > | ||
| <div className={styles.sizeSlider}> |
There was a problem hiding this comment.
Maybe it's just me, but it seems the two sliders on top of each other cause the header to have some empty space 🤔
What about having them side by side?
There was a problem hiding this comment.
Could be complicated but I think we should consolidate everything into the section header and make it sticky (as per #3443 (review)). WDYT?
There was a problem hiding this comment.
I think it'll make the whole thing too bulky. I also like the fact that when we collapse the section, we don't see the sliders anymore. It's more of a distraction otherwise. Having them side by side isn't a bad idea. But I'll make the headers sticky as well.
mattseddon
left a comment
mattseddon
left a comment
There was a problem hiding this comment.
Change LGTM.
One issue that I managed to create was y-axis label overflow (cc @julieg18):
Screen.Recording.2023-03-13.at.8.47.32.am.mov
| REGULAR = 2, | ||
| SQUARE = 3, | ||
| VERTICAL_NORMAL = 4, | ||
| VERTICAL_LARGER = 5 |
There was a problem hiding this comment.
[Q] Could these be the aspect ratios if that is what they refer to?
There was a problem hiding this comment.
Would we gain/lose anything by having this as PlotAspectRatio?
There was a problem hiding this comment.
First iteration used aspect ratios instead, but it made it more complicated to hold fractions instead of numbers. It makes it way easier to change the aspect ratio at the end of the chain (stylesheet) than doing so inside the enum here.
You're right, I forgot that y title truncation is done using the plot width at the moment. Need to change it to use the height instead. |
|
Code Climate has analyzed commit 1ab4679 and detected 8 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 81.1% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.7% (0.0% change). View more on Code Climate. |
3 participants
Part of #2585
Screen.Recording.2023-03-08.at.3.42.06.PM.mov