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

feat(Slideover): handle top and bottom side #1834

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

emavitta
Copy link
Contributor

@emavitta emavitta commented Jun 3, 2024

πŸ”— Linked issue

Resolves #576

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This pr adds top and bottom to the side prop.
It's just a working proof of concepts, meaning i didn't care about the braking changes it involves. Adding new config keys (side) forced me to remove the original width. I'd like to discuss in this pr how to reconcile my changes in the best way possible. I think the news side config (along with the news sideType computed property) is a nice way to implement the change, but i'm open to everything.

Also, not discussed directly in the issue, i'd like to know if could be interesting adding a fullscreen reactive option, letting the slideover go fullscreen; this could be usefull in mobile mode, creating the drawer effect and letting the user enlarge the content on an action.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@benjamincanac benjamincanac changed the title feat(slideover)!: add top and bottom to side prop. feat(Slideover)!: handle top and bottom side Jun 3, 2024
@emavitta
Copy link
Contributor Author

emavitta commented Jun 3, 2024

I was thinking that it would be possibile to leave the width (and add height) for the default horizontal version, and add a width-vertical along with the height-verticalkey for the vertical version; this way the default config could be left untouched. I don't like very much the solution out of a custom object but it maybe the simplest way to keep it from breaking.

All of this if the solution i provided is good, meaning you can specify 2 different width options for the vertical and the horizontal, and have a default optimized config for both of them; the other solution is just provide example of how to customize the width key if in vertical mode but it sounds wrong to me.

@benjamincanac
Copy link
Member

I think the easiest way to prevent breaking changes would be to keep the width key with w-screen max-w-md and add an height key with h-screen max-h-96 and hard-code the w-full / h-full. This way, you could do something like this in the template: sideType === 'horizontal' ? [ui.width, 'h-full'] : [ui.height, 'w-full'].

@emavitta
Copy link
Contributor Author

emavitta commented Jun 4, 2024

Clever! I'll update the pr.....
I tend to over config because you never know and too many times i was right; but i cannot imagine any reason why you should have a slideover from bottom not full width.

@emavitta
Copy link
Contributor Author

emavitta commented Jun 4, 2024

It seems to work nicely.
In the issue, ther's this comment that i was wondering about; seems something reasonable, but at the same time it may be an unexpected behaviour. Do you have an opinion about that? do you think it may be another issue?

@emavitta emavitta changed the title feat(Slideover)!: handle top and bottom side feat(Slideover): handle top and bottom side Jun 4, 2024
@benjamincanac
Copy link
Member

@emavitta Thinking about it, this can be achieved using useBreakpoints to change the side based on the breakpoint.

Thanks for the PR! 😊

@benjamincanac benjamincanac merged commit 50ad14f into nuxt:dev Jun 5, 2024
2 checks passed
@emavitta
Copy link
Contributor Author

emavitta commented Jun 5, 2024

That's what i was thinking, it is easy to implement with useBreakpoint and it may be confusing as a default behaviour.
Thanks

@SatheesA
Copy link

SatheesA commented Jun 8, 2024

I see that the documentation on the site says it supports "top" and "bottom" but that is not possible to do in my repo.
CleanShot 2024-06-08 at 08 39 21@2x

as of now only "left" and "right" works for me

I see that the nuxt/ui version is 2.16 but I only have nuxt/ui-pro and no package named nuxt/ui. How do I update so this works for me?

Copy link
Member

@SatheesA It hasn't been released yet. However, you can try it out by using the edge package: https://ui.nuxt.com/getting-started/installation#edge

@SatheesA
Copy link

SatheesA commented Jun 9, 2024

Ok thanks 😊. But does the doc not reflect the current version in production? I have not picked the edge version and looking at 2.16.0

https://ui.nuxt.com/components/slideover#props
It is listed as an valid option here. And can i control which version I am using since I only have nuxt pro ui in my package.json? Sorry if it is not the right place to ask questions

Copy link
Member

Yeah unfortunately the components props are parsed on build and the docs deploy from the dev branch which leads to this. I'll try to find a better way for v3, sorry about that!

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.

[Slideover] Add direction from Bottom and top
3 participants