Percentage slider -> Aligned min/max dividers #427

fti-rchaniya · 2023-10-19T12:04:37Z

Description
This PR has following enhancement in the PercentageSlider component:

In our current component, it was noticed in the demo by @h2suzuki san that the alignment of the min (0%) and max(100%) values are not proper as shown in below attached snapshot,
So, I have aligned min (0%) and max(100%) values as shown in the below attached snapshot,

atomworks

At the moment this is using a nested terniary which makes it very hard to read. It's better to break this logic in to different conditionals so it's easier to identify the cases it's being used for, perhaps instance (or line) for the normal case and then another for the end cases.

Also, I would like to add a prop to this component to make this alignment of the end numbers option (as there are cases where it will need to be aligned as it is originally). Thank you.

…the percentage slider

fti-rchaniya · 2023-11-27T11:06:36Z

At the moment this is using a nested terniary which makes it very hard to read. It's better to break this logic in to different conditionals so it's easier to identify the cases it's being used for, perhaps instance (or line) for the normal case and then another for the end cases.

Also, I would like to add a prop to this component to make this alignment of the end numbers option (as there are cases where it will need to be aligned as it is originally). Thank you.

Thank you for your feedback. I appreciate your suggestion to improve readability by breaking down the nested ternary logic. I have refactored the code to use separate conditionals for better clarity, distinguishing between normal and end cases.

Additionally, I have implemented a new prop for the component to provide an option for aligning the end numbers, as you've suggested. This will allow more flexibility in controlling the alignment based on specific use cases.
Kindly verify changes and let me know if it's match your suggestion
Thank you.

isacoder

I have only one comment in the new variable, since we want to continue to support the feature for previous versions let's make it optional.

Since the SliderInput base is modified please also update the story of the SliderInput

isacoder · 2023-12-07T14:39:36Z

src/Form/molecules/PercentageSlider.tsx

@@ -53,6 +53,7 @@ interface IPercentageSliderProps {
  inputCallback?: (value: number) => void
  updateThumbColor?:  (value: number) => IFeedbackColor
  updateTitle?: (value: number) => string
+  isCenterAlignedEndNum: boolean


Since this is a new prop, you might want to make it optional an also set it to false, since previously it was not center.

@isacoder, Feedback changes have been implemented, kindly review once again and let me know if any change is required.

Thanks

atomworks

Once Isabel's feedback has been followed, I'm happy to approve this

isacoder

Hi! Thank you for updating the Slider Input Story, it's helping with this review.
I see that the last value in the example is very missed aligned. Please modify the code to cover this case too.

fti-rchaniya · 2023-12-11T10:40:17Z

Hi! Thank you for updating the Slider Input Story, it's helping with this review. I see that the last value in the example is very missed aligned. Please modify the code to cover this case too.

@isacoder san, I have pushed requested changes, can you please check it once and let me know if any changes needed
Thanks:woman-bowing:

isacoder · 2023-12-15T05:17:52Z

Hi! Thank you for updating the Slider Input Story, it's helping with this review. I see that the last value in the example is very missed aligned. Please modify the code to cover this case too.

Hi! Thank you for updating the Slider Input Story, it's helping with this review. I see that the last value in the example is very missed aligned. Please modify the code to cover this case too.

@isacoder san, I have pushed requested changes, can you please check it once and let me know if any changes needed Thanks:woman-bowing:

I did a full review of the component, since the update is not working as expected.
The Slider component was already capable of having the marks centered. This functionality was not available to UIKIT users because of design concerns. However since now is a requirement I think adding the prop to make it available is fine.

I think is better to use the logic that was already in the component since adding extra logic will increase maintenance of the component. I have made a commit to revert the previous logic.

Also Updated the name of the variable isCenterAlignedEndNum -> allMarksCentered to reflect this change.

@fti-rchaniya if you don't have a problem with this update we can move on with this PR

fti-rchaniya · 2023-12-18T10:35:43Z

Hi! Thank you for updating the Slider Input Story, it's helping with this review. I see that the last value in the example is very missed aligned. Please modify the code to cover this case too.

Hi! Thank you for updating the Slider Input Story, it's helping with this review. I see that the last value in the example is very missed aligned. Please modify the code to cover this case too.

@isacoder san, I have pushed requested changes, can you please check it once and let me know if any changes needed Thanks:woman-bowing:

I did a full review of the component, since the update is not working as expected. The Slider component was already capable of having the marks centered. This functionality was not available to UIKIT users because of design concerns. However since now is a requirement I think adding the prop to make it available is fine.

I think is better to use the logic that was already in the component since adding extra logic will increase maintenance of the component. I have made a commit to revert the previous logic.

Also Updated the name of the variable isCenterAlignedEndNum -> allMarksCentered to reflect this change.

@fti-rchaniya if you don't have a problem with this update we can move on with this PR

@isacoder san,

Thank you for your detailed review and efforts on addressing the update issue with the Slider component.

I agree with your decision to leverage the existing logic in the component rather than introducing additional logic to avoid increasing maintenance overhead.
I support the proposed changes, and let's move forward with this pull request.
Thanks

isacoder

Updated and approved

Percentage slider -> Aligned min/max dividers

aa14ec0

fti-rchaniya requested review from fti-bcarpenter, a user and fti-pfarne October 19, 2023 12:05

ghost approved these changes Oct 19, 2023

View reviewed changes

fti-bcarpenter approved these changes Oct 19, 2023

View reviewed changes

fti-rchaniya requested a review from atomworks November 22, 2023 10:59

atomworks requested changes Nov 22, 2023

View reviewed changes

Added a prop to conditionally handle alignment of the end numbers of …

7f3c1a8

…the percentage slider

fti-rchaniya requested review from fti-bcarpenter, fti-skumar and atomworks November 27, 2023 11:01

atomworks requested a review from isacoder December 5, 2023 09:34

isacoder requested changes Dec 7, 2023

View reviewed changes

atomworks reviewed Dec 8, 2023

View reviewed changes

Feedback changes have been implemented

c6a04e6

fti-rchaniya requested review from atomworks and isacoder December 8, 2023 11:11

isacoder requested changes Dec 11, 2023

View reviewed changes

isacoder self-requested a review December 11, 2023 04:32

Last value misalignment fixed for Slider Input component

0de3b48

isacoder added 3 commits December 15, 2023 12:26

Update mark logic to allow center at the extremities

e384e98

Update wording in slider input example

6fe9303

Update padding in Title percentage when marks are centered

c6c478d

isacoder requested a review from a user December 15, 2023 05:19

atomworks approved these changes Dec 19, 2023

View reviewed changes

isacoder approved these changes Dec 19, 2023

View reviewed changes

isacoder merged commit 82e9177 into main-v1.x Dec 19, 2023
1 check passed

isacoder deleted the refactor/percentage_slider_divider_placement branch December 19, 2023 14:52

isacoder mentioned this pull request Jan 18, 2024

Main v1 integration into version 2 #433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Percentage slider -> Aligned min/max dividers #427

Percentage slider -> Aligned min/max dividers #427

fti-rchaniya commented Oct 19, 2023

atomworks left a comment

fti-rchaniya commented Nov 27, 2023

isacoder left a comment

isacoder Dec 7, 2023

fti-rchaniya Dec 8, 2023

atomworks left a comment

isacoder left a comment

fti-rchaniya commented Dec 11, 2023

isacoder commented Dec 15, 2023

fti-rchaniya commented Dec 18, 2023

isacoder left a comment

Percentage slider -> Aligned min/max dividers #427

Percentage slider -> Aligned min/max dividers #427

Conversation

fti-rchaniya commented Oct 19, 2023

atomworks left a comment

Choose a reason for hiding this comment

fti-rchaniya commented Nov 27, 2023

isacoder left a comment

Choose a reason for hiding this comment

isacoder Dec 7, 2023

Choose a reason for hiding this comment

fti-rchaniya Dec 8, 2023

Choose a reason for hiding this comment

atomworks left a comment

Choose a reason for hiding this comment

isacoder left a comment

Choose a reason for hiding this comment

fti-rchaniya commented Dec 11, 2023

isacoder commented Dec 15, 2023

fti-rchaniya commented Dec 18, 2023

isacoder left a comment

Choose a reason for hiding this comment