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.

Sign up for GitHub

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

Jump to bottom

[Loom-1535] - add form-test to bpkSlider #3579

Merged
merged 4 commits into from
Aug 12, 2024
Merged

[Loom-1535] - add form-test to bpkSlider #3579

merged 4 commits into from
Aug 12, 2024

Conversation

Parisistan
Copy link
Contributor

Adding a form-test to our slider component to check if it renders correctly.

Remember to include the following changes:

  • Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g [KOA-123][BpkButton] Updating the colour
  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Accessibility tests
    • The following checks were performed:
      • Ability to navigate using a keyboard only
      • Zoom functionality (Deque University explanation):
        • The page SHOULD be functional AND readable when only the text is magnified to 200% of its initial size
        • Pages must reflow as zoom increases up to 400% so that content continues to be presented in only one column i.e. Content MUST NOT require scrolling in two directions (both vertically and horizontally)
      • Ability to navigate using a screen reader only
  • Storybook examples created/updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@Parisistan Parisistan added enhancement minor Non breaking change labels Aug 7, 2024
@Parisistan Parisistan force-pushed the LOOM-1535-enhance-bpkSlider branch from 4d419ca to d44f433 Compare August 7, 2024 13:56
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2024

Visit https://backpack.github.io/storybook-prs/3579 to see this build running in a browser.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2024 •
edited
Loading

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 03f4e5a

@Parisistan Parisistan marked this pull request as ready for review August 7, 2024 14:07
@Parisistan Parisistan changed the title Loom-1535: enhance bpk slider [Loom-1535] - add form-test to bpkSlider Aug 7, 2024
mungodewar
mungodewar reviewed Aug 8, 2024
View reviewed changes
packages/bpk-component-slider/src/form-test.tsx Outdated
ariaLabel={['slider']}
ariaValuetext={['min', 'max']}
data-testid="mySlider"
onChange={(value) => setSliderValue(Array.isArray(value) ? value : [value])}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the value sometimes within an array and sometimes not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because sometimes there aren't a min and max value that could both change and only one side of the slider is changeable. So I wanted one of the tests to check for when both min, max can change to cover various types. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

@mungodewar mungodewar Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think I see, so if the sliderValue={[20,80]} then the bottom "two thumb" slider will be shown?

I think the "two thumb" test case is nice - it's certainly the most complex of the two scenarios. Given that we are in control of the test (and if the slider is two or one) I think we should remove the isArray code in order to make it clearer what is expected.

Copy link
Contributor Author

@Parisistan Parisistan Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

so if the sliderValue={[20,80]} then the bottom "two thumb" slider will be shown?

yes that is right!
And okay, I can make it be clear in the description too so there is no confusion for users. :)

packages/bpk-component-slider/src/form-test.tsx Outdated Show resolved Hide resolved
packages/bpk-component-slider/src/form-test.tsx Outdated
expect(handleChange).toHaveBeenCalledTimes(1);
});

it('should emit change event when both sides of slider value are changed', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I think this test has overlap with "should emit change event when slider value is changed" & thus I think you only need one of them & I think this test is better as it tests both slider value changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah fair!

@Parisistan Parisistan force-pushed the LOOM-1535-enhance-bpkSlider branch 2 times, most recently from c113ff1 to 3a0f0f0 Compare August 8, 2024 10:01
@Parisistan Parisistan requested a review from mungodewar August 8, 2024 10:02
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 8, 2024

Visit https://backpack.github.io/storybook-prs/3579 to see this build running in a browser.

@Parisistan Parisistan force-pushed the LOOM-1535-enhance-bpkSlider branch from 3a0f0f0 to 889eeb5 Compare August 9, 2024 13:37
@Parisistan Parisistan force-pushed the LOOM-1535-enhance-bpkSlider branch from 889eeb5 to 03f4e5a Compare August 9, 2024 13:37
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 9, 2024

Visit https://backpack.github.io/storybook-prs/3579 to see this build running in a browser.

mungodewar
mungodewar approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@mungodewar mungodewar left a comment

Choose a reason for hiding this comment

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

LGTM - nice 🎉

@Parisistan Parisistan merged commit e25ea97 into main Aug 12, 2024
9 checks passed
@Parisistan Parisistan deleted the LOOM-1535-enhance-bpkSlider branch August 12, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mungodewar mungodewar mungodewar approved these changes

Assignees
No one assigned
Labels
enhancement minor Non breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Parisistan @mungodewar