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

[#1725] VaSplit component implementation #2068

Merged
merged 23 commits into from
Aug 9, 2022
Merged

[#1725] VaSplit component implementation #2068

merged 23 commits into from
Aug 9, 2022

Conversation

aluarius
Copy link
Contributor

Close: #1725

Description

New split component implementation.

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)

@aluarius aluarius self-assigned this Jul 10, 2022
m0ksem
m0ksem approved these changes Jul 10, 2022
View reviewed changes
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 have real-world example (like Sidebar and App content.)

Have you tested this on phone? You will need to listen for touchstart, touchend events.

packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
@m0ksem m0ksem added this to the 1.5.0 milestone Jul 10, 2022
rustem-nasyrov
rustem-nasyrov requested changes Jul 11, 2022
View reviewed changes
Copy link
Collaborator

@rustem-nasyrov rustem-nasyrov left a comment

Choose a reason for hiding this comment

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

Looks awesome! I have a little suggestion: if the cursor goes out of the splitter container we lose the 'draggable' state, would be better if the user can move the splitter out of the container.

packages/docs/src/page-configs/ui-elements/split/examples/CustomGrabber.vue Outdated Show resolved Hide resolved
packages/docs/src/page-configs/ui-elements/split/examples/Disabled.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.demo.vue Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
@aluarius aluarius requested a review from rustem-nasyrov July 11, 2022 15:39
aluarius added 2 commits July 12, 2022 23:20
@aluarius
fix:deleted redundant events
ab71406 
fix:minor fix
fix:redundant limits prop changed to number
@aluarius
fix:deleted redundant events
5f2b1bc 
fix:minor fix
fix:redundant limits prop changed to number
@m0ksem m0ksem added the component Is a new component or part of existing one label Jul 14, 2022
aluarius added 2 commits July 15, 2022 15:39
@aluarius
fix:changed wrong modelValue value warning to existing function
baa01ab
@aluarius
feat:limits prop logic rework
d150d69 
feat:added resize observer to the container (to recalc limits)
feat:docs updates
@aluarius aluarius requested a review from m0ksem July 18, 2022 13:45
RVitaly1978
RVitaly1978 requested changes Jul 18, 2022
View reviewed changes
packages/docs/src/page-configs/ui-elements/split/examples/CustomGrabber.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
@aluarius aluarius requested a review from RVitaly1978 July 19, 2022 11:51
RVitaly1978
RVitaly1978 requested changes Jul 19, 2022
View reviewed changes
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
@aluarius aluarius requested a review from RVitaly1978 July 20, 2022 10:19
@aluarius
Merge branch 'release/1.5.0' into feat/split-component
cb877fb 
# Conflicts:
#	packages/docs/src/locales/en/en.json
#	packages/docs/src/locales/ru/ru.json
@aluarius aluarius mentioned this pull request Aug 2, 2022
Closed
@aluarius
Copy link
Contributor Author

aluarius commented Aug 3, 2022 •
edited
Loading

From discussion with Vitaly:

  • should we make min-max as snapping marks in case snapping-range is set;
  • should we give to user an ability to change snapping-range (few ways to break component via this prop).

m0ksem
m0ksem approved these changes Aug 3, 2022
View reviewed changes
packages/ui/src/components/va-split/VaSplit.vue
})

switch (measureValue) {
case '%':
Copy link
Collaborator

Choose a reason for hiding this comment

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

v.endsWith('%')
v.endsWith('px')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to parse value anyway. So, this suggestion isn't suitable in my opinion.

packages/ui/src/components/va-split/VaSplit.vue
Comment on lines +126 to +130
case '':
return 100
default:
warn('Invalid limits measure!')
return 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

default:
if (v !== Number(v)) warn('puk')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

packages/ui/src/components/va-split/VaSplit.vue Outdated Show resolved Hide resolved
packages/ui/src/components/va-split/useSplitDragger.ts Outdated Show resolved Hide resolved
@m0ksem
Copy link
Collaborator

m0ksem commented Aug 3, 2022

Can we also have some specs like this? We will put it later under documentation page. It takes too much time to understand how it works...

@aluarius aluarius merged commit 077c6bf into epicmaxco:release/1.5.0 Aug 9, 2022
@aluarius aluarius mentioned this pull request Aug 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvae asvae asvae left review comments

@RVitaly1978 RVitaly1978 RVitaly1978 approved these changes

@rustem-nasyrov rustem-nasyrov rustem-nasyrov approved these changes

@m0ksem m0ksem m0ksem approved these changes

@Derranion Derranion Awaiting requested review from Derranion

Assignees

@aluarius aluarius

Labels
component Is a new component or part of existing one
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@aluarius @m0ksem @asvae @rustem-nasyrov @RVitaly1978