Add line width setting for wizard line charts#3389
Add line width setting for wizard line charts#3389arsen-afaunov wants to merge 7 commits intomainfrom
Conversation
|
|
|
|
📦 Statoscope quick diff with main-branch: ⏱ Build time: -2.1 sec (-3.57%) ⚖️ Initial size: 1.91 kb (0.04%) 🕵️ Validation errors: 0 Full Statoscope report could be found here |
arsen-afaunov
force-pushed
the
CHARTS-10226-add-line-width-setting-for-wizard-line-charts
branch
2 times, most recently
from
5e795d4 to
8b0c9ad
Compare
arsen-afaunov
requested review from
kuzmadom and
mournfulCoroner
and removed request for
mournfulCoroner
If you enter the line width value 2000 for example. It would be necessary to limit
|
|
Line width setting is not remembered in dialog Steps to Reproduce: Expected Result: the line width field in the dialog displays the value that was previously set by the user. |
kuzmadom
left a comment
kuzmadom
left a comment
There was a problem hiding this comment.
The Line width parameter is incorrectly displayed and can be modified in the settings dialog for a Scatter-type chart. Since scatter plots typically use individual markers (points) and not connected lines, this setting should not be applicable and should be hidden
| expect(lines[0]).toEqual(DashArrayLineType[LineShapeType.Dash]); | ||
| }); | ||
|
|
||
| datalensTest('User can change line width', async ({page}: {page: Page}) => { |
There was a problem hiding this comment.
You need to move the test files to the tests/opensource-suites directory and update all dependent paths, particularly those pointing to test data and resources
arsen-afaunov
force-pushed
the
CHARTS-10226-add-line-width-setting-for-wizard-line-charts
branch
4 times, most recently
from
d845cd8 to
23b46a5
Compare
| -moz-appearance: textfield; | ||
| } | ||
| /* stylelint-enable */ | ||
| &__text-input > span { |
There was a problem hiding this comment.
It's better to put something specific on the span. It's very fragile right now
| qa?: string; | ||
| className?: string; | ||
| buttonClassName?: string; | ||
| inputClassName?: string; |
There was a problem hiding this comment.
do we really need specific styles for all this? Is it possible to change styles everywhere to something the same?
| commitValue(min); | ||
| } else { | ||
| commitValue(parsed - 1); | ||
| } |
There was a problem hiding this comment.
This block of code is repeated several times in the file, in almost the same form. Maybe something can be done about it?
| private cancelButton = '.dialog-shapes .g-dialog-footer__button_action_cancel'; | ||
| private valueLabelSelector = '.dialog-shapes .values-list__value-label'; | ||
| private applyButton = '.dialog-shapes .g-dialog-footer__button_action_apply'; | ||
| private lineWidthSelectControl = '.dialog-shapes .dl-line-width-select__control'; |
There was a problem hiding this comment.
This can only be used when there is no other way out, for example, to obtain elements from third-party libraries. If it's something of ours, it's better to add a qa attribute
arsen-afaunov
force-pushed
the
CHARTS-10226-add-line-width-setting-for-wizard-line-charts
branch
12 times, most recently
from
eb63e6d to
19013e0
Compare
arsen-afaunov
force-pushed
the
CHARTS-10226-add-line-width-setting-for-wizard-line-charts
branch
from
19013e0 to
b1a0078
Compare
arsen-afaunov
force-pushed
the
CHARTS-10226-add-line-width-setting-for-wizard-line-charts
branch
from
b1a0078 to
b25158f
Compare
|
i18n-check The following components have not been translatedWait for the reviewers to check your changes in Weblate and try running github action again. |
|
i18n-check The following components have not been translatedWait for the reviewers to check your changes in Weblate and try running github action again. |
| onChartLineWidthChange: (nextLineWidth: string) => void; | ||
| }; | ||
|
|
||
| export const DialogShapesChartSettingsTab: React.FC<Props> = ({ |
There was a problem hiding this comment.
It is better not to use React.FC
| @@ -13,53 +50,103 @@ interface NumberInputProps { | |||
| qa?: string; | |||
| } | |||
|
|
|||
| const b = block('wizard-number-input'); | |||
| const DEFAULT_VALUE = 0; | |||
|
|
|||
| const NumberInput: React.FC<NumberInputProps> = ({ | |||
There was a problem hiding this comment.
if this component needs to be used outside of NumberFormatSettings, it is better to move it to the shared components folder
| [STEP_BUTTON_DIRECTION.Minus]: {icon: Minus, pin: 'round-brick'}, | ||
| }; | ||
|
|
||
| const b = block('wizard-number-input'); |
There was a problem hiding this comment.
There doesn't seem to be any specifics in the component specifically for wizard? It is better not to add unnecessary names to the classes in this case - it will be more difficult to reuse later somewhere else
| </Button> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
It looks like you can put StepButton in a separate file - there's already quite a lot besides the layout
| export type V15ShapesConfig = { | ||
| mountedShapes?: Record<string, string>; | ||
| mountedShapesLineWidths?: Record<string, number>; | ||
| chartLineWidth?: number; |
There was a problem hiding this comment.
Is it possible to refer to it without using a chart? The whole config is already related to the chart, so this doesn’t add any meaning.
| // Determine line width: use individual line width if set, otherwise fall back to chart-level width | ||
| let lineWidth: number | undefined; | ||
|
|
||
| if (title && shapesConfig?.mountedShapesLineWidths?.[title]) { |
There was a problem hiding this comment.
what if title is an empty string (or null)? Is everything going to be okay?
|
i18n-check The following components have not been translatedWait for the reviewers to check your changes in Weblate and try running github action again. |
4 participants
No description provided.