Skip to content
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

Resize comparison table with slider #3480

Merged
merged 6 commits into from
Mar 17, 2023
Merged

Resize comparison table with slider #3480

merged 6 commits into from
Mar 17, 2023

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Mar 16, 2023

Part of #2585

Screen.Recording.2023-03-16.at.1.04.03.PM.mov

@sroy3 sroy3 added the product PR that affects product label Mar 16, 2023
@sroy3 sroy3 self-assigned this Mar 16, 2023
@sroy3 sroy3 changed the title Resize comparison tablw with slider Resize comparison table with slider Mar 16, 2023
import { PlotsContainer } from '../PlotsContainer'
import { PlotsState } from '../../store'

export const ComparisonTableWrapper: React.FC = () => {
const { nbItemsPerRow, isCollapsed, height } = useSelector(
const { width, isCollapsed, height, plots } = useSelector(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 3 locations. Consider refactoring.

@@ -6,6 +6,12 @@ export const withScale = (scale: number) =>
HTMLDivElement
>)

export const withVariant = (variant: number) =>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar blocks of code found in 2 locations. Consider refactoring.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 16, 2023

Sorry for the renaming, it just made more sense that way. The main parts of the change are found in ComparisonTableWrapper.tsx, ComparisonTable.tsx, PlotsContainer.tsx and plots/components/styles.modules.scss.

@sroy3 sroy3 marked this pull request as ready for review March 16, 2023 17:44
@sroy3 sroy3 closed this Mar 16, 2023
@sroy3 sroy3 reopened this Mar 16, 2023
@mattseddon
Copy link
Member

mattseddon commented Mar 17, 2023

Sticky slider collides with comparison header:

image

Can the slider/ribbon fill the space on the RHS?

image

Will both of the above be fixed by the section having its own scrollbar?

Edit: With pinned column

image

Copy link
Member

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks fine outside of previously mentioned issue

@@ -56,7 +56,7 @@ export type CustomPlotsOrderValue = { metric: string; param: string }
export class PlotsModel extends ModelWithPersistence {
private readonly experiments: Experiments

private nbItemsPerRow: Record<PlotsSection, number>
private nbItemsPerRowOrWidth: Record<PlotsSection, number>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Do these need to be stored together?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was debating this, but they function the exact same way it's just that one cannot be called nbItemsPerRow. Separating them would probably mean checking whether it has one property or another multiple times.

@sroy3
Copy link
Contributor Author

sroy3 commented Mar 17, 2023

Sticky slider collides with comparison header:

image

Can the slider/ribbon fill the space on the RHS?

image

Will both of the above be fixed by the section having its own scrollbar?

Edit: With pinned column

image

The sticky header colliding is easy to fix now. Same for the pinned column. The header taking the full width will be easier with the local scrollbar (next item on my list), else we'll end up with sliders past smaller sections.

@sroy3 sroy3 enabled auto-merge (squash) March 17, 2023 13:41
@codeclimate
Copy link

codeclimate bot commented Mar 17, 2023

Code Climate has analyzed commit b524c59 and detected 7 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 7

The test coverage on the diff in this pull request is 86.9% (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.

@sroy3 sroy3 merged commit 4c2d7d1 into main Mar 17, 2023
@sroy3 sroy3 deleted the comparison-resize branch March 17, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants