Skip to content

Conversation

@arteria32
Copy link
Contributor

No description provided.

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui
Copy link

gravity-ui bot commented Nov 25, 2025

Visual Tests Report is ready.

@arteria32 arteria32 changed the title Feat(Legend): add left and right options to legend.position config feat(Legend): add left and right options to legend.position config Nov 26, 2025
@arteria32 arteria32 marked this pull request as ready for review November 26, 2025 05:40
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@korvin89 @kuzmadom
Can you tell me if there are any requirements for centering the legend along the vertical axis?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think an additional field such as verticalAlign is suitable for vertical positioning of the legend (similar to horizontal align?: 'left' | 'center' | 'right';)
But this can (should) be done in a separate pr. For the legend to be positioned on the left/right, the current top-only positioning is enough - this is ok in my opinion

Copy link
Contributor Author

@arteria32 arteria32 Nov 27, 2025

Choose a reason for hiding this comment

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

But this can (should) be done in a separate pr.

OK, I will try to implement this in the next pr.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does there seem to be enough space from below? I would have expected to see more series in legend to take up all the available space

Copy link
Contributor Author

@arteria32 arteria32 Nov 27, 2025

Choose a reason for hiding this comment

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

I would have expected to see more series in legend to take up all the available space

I'll take a look at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

legend?.colorScale?.domain ?? getDomainForContinuousColorScale({series});
} else {
height += lineHeight;
legendWidth = get(legend, 'width', DEFAULT_LEGEND_WIDTH);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to use the preset fixed value setting for the legend on the right/left. As a user, I would expect that the width will be calculated depending on the width of the legend elements - that is, the series name plus the symbol. But if the width is set explicitly, then OK, you can limit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@arteria32
Copy link
Contributor Author

@kuzmadom
Can you review it again, please?

width: 200,
};

export const DEFAULT_LEGEND_WIDTH = 80;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like it's not in use

kuzmadom
kuzmadom previously approved these changes Dec 8, 2025
@arteria32 arteria32 force-pushed the feat/add-left-right-position branch 2 times, most recently from f507195 to 63f5644 Compare December 9, 2025 06:34
@arteria32
Copy link
Contributor Author

@kuzmadom
Hi, can you help me with failed test?
Where am I going wrong with the legend positioning rules?:
In the main branch, the legend with justify-content: center and align-items: center (test case: grouped legend items) have different absolute left and right margins.
Screenshot 2025-12-09 at 09 51 45
Maybe, I used an incorrect anchor point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants