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

Stories rework: VaDropdown #4044

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

Fsss126
Copy link
Contributor

@Fsss126 Fsss126 commented Nov 28, 2023

See #3676.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

@Fsss126 Fsss126 requested review from asvae and m0ksem November 28, 2023 23:39
@Fsss126 Fsss126 mentioned this pull request Nov 28, 2023
75 tasks
Copy link
Collaborator

@m0ksem m0ksem left a comment

Choose a reason for hiding this comment

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

Would be nice to cover stories with tests.

packages/ui/.storybook/storybook-main.scss Outdated Show resolved Hide resolved
Copy link
Member

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Great work!

Left some story suggestions. We can merge after fix!

keepAnchorWidth
>
<template #anchor>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

div needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it

components: { VaDropdown, VaValue, VaDropdownContent },
template: `
<VaValue #default="v" :default-value="true">
<button @click="v.value = !v.value">{{ v.value ? 'Short' : 'Long' }}</button>
Copy link
Member

Choose a reason for hiding this comment

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

No switches without big need.

Just use 2 examples in one story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it

`,
})

export const Cursor = () => ({
Copy link
Member

Choose a reason for hiding this comment

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

Trigger? Has nothing to do with cursor.

See border implementation for good examples on how to implement this

http://localhost:6006/?path=/story/vaalert--border

notice default prop is not set in story - allows us to catch default regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How border relates to this? Didn't get 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.

Removed trigger

`,
})

export const AnchorBoundProps = () => ({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const AnchorBoundProps = () => ({
export const AnchorSlotProps = () => ({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it

<div style="height: 1000px">
<va-dropdown>
<template #anchor>
<va-button>Anchor</va-button>
Copy link
Member

Choose a reason for hiding this comment

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

let's either use va-button everywhere or nowhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it

`,
})

export const Autoplacement: StoryFn = () => ({
Copy link
Member

Choose a reason for hiding this comment

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

Need an example for default autoplacement behavior.

It's not clear to me how using autoplacementTargetRef differs from not using 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.

Default autoplacement behaviour without target doesn't seem to handle overflow properly and dropdown gets positioned outside of scroll parent. Is this needed to be showcased? I thought that in this case target needs to be specified for everything to work correctly and I doubt that improper usage should be shown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is current behaviour without specifying the target considered a bug?

@Fsss126 Fsss126 force-pushed the stories-rework-va-dropdown branch from c9f8f2e to f9aa185 Compare December 1, 2023 13:21
@Fsss126 Fsss126 requested review from asvae and m0ksem December 1, 2023 13:21
Copy link
Member

@asvae asvae left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@asvae asvae merged commit a5439fa into epicmaxco:develop Dec 8, 2023
2 checks passed
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.

3 participants