WIP: feat: route modals everywhere #2735

Closed
dpschen wants to merge 15 commits from dpschen/frontend:feature/route-modals-everywhere into main
Member

This enables route modals everywhere, where possible.

This is a major usability improvement, because you can go back to the route where you came from much faster.

This enables route modals everywhere, where possible. This is a major usability improvement, because you can go back to the route where you came from much faster.
dpschen added the
kind/feature
label 2022-11-21 12:35:52 +00:00
dpschen reviewed 2022-11-21 12:58:58 +00:00
@ -0,0 +1,21 @@
<!-- https://github.com/vuejs/rfcs/pull/449 -->
Author
Member

This component makes it possible to optionally wrap around another component. If the is prop defines a component it will be rendered as a wrapper around the slot content. If the is prop is undefined there won't be any wrapper.

This component makes it possible to optionally wrap around another component. If the `is` prop defines a component it will be rendered as a wrapper around the slot content. If the `is` prop is undefined there won't be any wrapper.
dpschen marked this conversation as resolved
@ -0,0 +10,4 @@
defineProps<{ is: any }>()
const attrs = useAttrs()
console.log(JSON.parse(JSON.stringify(attrs)))
Author
Member

Remove this

Remove this
dpschen marked this conversation as resolved
@ -40,3 +40,2 @@
<modal
:enabled="Boolean(currentModal)"
<component
Author
Member

Every modal component now has to provide it's own modal.

This makes it possible for us to have different modal implementations. Currently we do this by changing the type prop of the modal. It might be easier for us if we create something like a BaseModal to abstract the general modal functionality and then use that to create styled Modals with specific functionality. For example we coudl create a dedicated Dialog modal (this might actually be the same as the current create-edit, I'm not sure here if that was the intended use @konrad).

Every modal component now has to provide it's own modal. This makes it possible for us to have different modal implementations. Currently we do this by changing the `type` prop of the modal. It might be easier for us if we create something like a `BaseModal` to abstract the general modal functionality and then use that to create styled Modals with specific functionality. For example we coudl create a dedicated `Dialog` modal (this might actually be the same as the current `create-edit`, I'm not sure here if that was the intended use @konrad).

That sounds like it could be a good idea.

IIRC my main goal with the create-edit component was to be able to easily re-use a shell for creating or editing.

That sounds like it could be a good idea. IIRC my main goal with the `create-edit` component was to be able to easily re-use a shell for creating or editing.
@ -1,38 +1,38 @@
<template>
<modal @close="$router.back()" :overflow="true" :wide="wide">
<modal :overflow="true" :wide="wide" #default="{ onClose }">
Author
Member

I removed the @close event definitions on all internal modals because what happens when that event get's fired should instead be defined by the parent. By removing this the parents onClose / @close should automatically be passed to the <modal>.
By passing the onClose then to the slot content, the slot is still able to use the method.

I removed the `@close` event definitions on all internal modals because what happens when that event get's fired should instead be defined by the parent. By removing this the parents `onClose` / `@close` should automatically be passed to the `<modal>`. By passing the `onClose` then to the slot content, the slot is still able to use the method.
@ -88,3 +89,1 @@
function primary() {
emit('create')
emit('primary')
function primary(onClose: () => void) {
Author
Member

We pass onClose as a callback parameter to the primary emit so that it can reuse the onClose after the primary action finished (unsure if that makes sense).

We pass `onClose` as a callback parameter to the `primary` emit so that it can reuse the `onClose` after the primary action finished (unsure if that makes sense).
@ -2,3 +2,3 @@
<Teleport to="body">
<!-- FIXME: transition should not be included in the modal -->
<CustomTransition :name="transitionName" appear>
<CustomTransition :name="transitionName" appear :appear-class="transitionName">
Author
Member

The appear transition is currently broken. Fix this.

The appear transition is currently broken. Fix this.
@ -98,2 +97,4 @@
scrollLock.value = props.enabled
})
function onClose() {
Author
Member

FIXME: either we use an implicit attrs.onClose or we explicit define a close emit. Both should not be mixed.

FIXME: either we use an implicit `attrs.onClose` or we explicit define a `close` emit. Both should not be mixed.
@ -45,3 +45,3 @@
const historyState = computed(() => route.fullPath && window.history.state)
if (historyState.value) {
if (historyState.value === undefined) {
Author
Member

Something was broken here: since we checked for historyState.value in the if condition it could never have a value in the else cause.

Something was broken here: since we checked for `historyState.value` in the `if` condition it could never have a value in the `else` cause.
@ -80,3 +80,3 @@
variant="tertiary"
class="is-danger"
@click.prevent.stop="removeBackground"
@click="removeBackground(onClose)"
Author
Member

We should not need to use .prevent and .stop with the vueuse onClickOutside.

We should not need to use `.prevent` and `.stop` with the vueuse `onClickOutside`.
@ -1,4 +1,5 @@
<template>
<OptionalWrapper :is="isModal && Modal" variant="scrolling">
Author
Member

By using the OptionalWrapper we can use components that can be used with or without Modal

By using the OptionalWrapper we can use components that can be used with or without Modal
@ -510,2 +515,3 @@
defineEmits(['close'])
const attrs = useAttrs()
console.log(JSON.parse(JSON.stringify(attrs)))
Author
Member

Remove log

Remove log
dpschen self-assigned this 2022-11-21 12:59:06 +00:00
dpschen force-pushed feature/route-modals-everywhere from eb6d239213 to 130620b077 2022-11-21 17:50:36 +00:00 Compare
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2735-feature-route-modals-everywhere--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2735-feature-route-modals-everywhere--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen force-pushed feature/route-modals-everywhere from 130620b077 to a6765638c1 2022-11-22 10:20:48 +00:00 Compare
dpschen added 11 commits 2022-11-22 20:46:21 +00:00
dpschen force-pushed feature/route-modals-everywhere from 87f8e66e34 to cd72db7287 2022-11-23 20:15:00 +00:00 Compare
dpschen force-pushed feature/route-modals-everywhere from cd72db7287 to de7d004c6e 2022-11-23 22:35:18 +00:00 Compare
Owner

Does it make sense to rebase this?

Does it make sense to rebase this?
Owner

Closing this due to inactivity. Please ping if you want to pick it up again.

Closing this due to inactivity. Please ping if you want to pick it up again.
konrad closed this pull request 2024-01-16 12:22:03 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.