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

refactor: modal implementation #10212

Merged
merged 6 commits into from
Dec 22, 2023
Merged

Conversation

JammingBen
Copy link
Collaborator

@JammingBen JammingBen commented Dec 19, 2023

Description

Remove the modal from vuex store and integrates it into pinia instead. This change completely reworks how to work with modals. The main benefits and changes are:

  • Proper typings
  • Allow stacking multiple modals
  • Modals close automatically on cancel/confirm
  • Modals automatically use the loading service on confirm
  • Streamline modal usage to have one way how to create modals

How to use it

Modals can be dispatched via the dispatchModal method like so:

const { dispatchModal } = useModals()

dispatchModal({
  title: 'Create folder',
  onConfirm: () => createFolder()
})

The modal will automatically be removed when either the cancel or the confirm button has been clicked and the resulting promise resolved successfully. If needed, a modal can be removed manually via the provided method removeModal.

It is also possible to register another modal while having a modal open. This will add the new modal to the top of the queue and immediately display it. After it closes, the previous modal will show again.

A modal which is present in the queue but not showing can be displayed via the method setModalActive.

Custom components

Modals can have custom components with custom properties:

const { dispatchModal } = useModals()

dispatchModal({
  title: 'Create folder',
  onConfirm: () => createFolder(),
  customComponent: CreateFolderComponent,
  customComponentAttrs: () => ({ space })
})

This will render the CreateFolderComponent inside the modal while still showing the cancel and confirm buttons at the bottom.

It is also possible to define the onConfirm and onCancel methods inside the component. They must be named onConfirm / onCancel and be exposed via expose({ onConfirm, onCancel }). Note that if these methods are defined in registerModal, they will take priority over defined methods in the component.

Modal actions can be hidden if you need to implement custom actions inside the component:

const { dispatchModal } = useModals()

dispatchModal({
  title: 'Create folder',
  onConfirm: () => createFolder(),
  customComponent: CreateFolderComponent,
  customComponentAttrs: () => ({ space }),
  hideActions: true
})

Now you can implement these actions on your own. Note that you should still expose onConfirm / onCancel via expose and not call them directly with your actions. Register additional event handlers on the buttons instead: emit('confirm') and emit('cancel'). This ensures that the methods get called via the wrapping component which provides the benefits such as automatically closing the modal after confirm or cancel.

Open questions / follow-ups

  • Move modal components in respective Modals folders
  • Rename modal components from ...Modal to ...ModalContent?
  • Add dev docs on how to use the modal?

Related Issue

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 change)
  • Technical debt
  • Tests

@JammingBen JammingBen self-assigned this Dec 19, 2023
Copy link

update-docs bot commented Dec 19, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@JammingBen JammingBen force-pushed the refactor-modal-implemetation branch 8 times, most recently from 99e4909 to 36bc288 Compare December 19, 2023 15:12
@JammingBen JammingBen marked this pull request as ready for review December 20, 2023 07:44
Copy link

sonarcloud bot commented Dec 22, 2023

Quality Gate Failed Quality Gate failed

Failed conditions

5 New Bugs (required ≤ 0)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

display: inline-grid;
grid-auto-flow: column;
grid-auto-columns: 1fr;
}
Copy link
Member

Choose a reason for hiding this comment

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

😍

await wrapper.vm.onConfirm()
try {
await wrapper.vm.onConfirm()
} catch (error) {}
Copy link
Member

Choose a reason for hiding this comment

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

Hum? What is this good for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we test for an error here the test would fail otherwise. So we wouldn't be able to set any expectations afterwards.

revert: event === 'beforeUnmount'
})
},

Copy link
Member

Choose a reason for hiding this comment

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

Is this now handled anywhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, but I didn't find a reason to keep it. Modals are being focused correctly due to the focus trap in OcModal.

await Promise.all([
this.store.dispatch('clearDynamicNavItems'),
this.store.dispatch('hideModal')
])
Copy link
Member

Choose a reason for hiding this comment

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

What did this even do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Closing a potentially open modal I guess 😄

state,
actions,
mutations
}
Copy link
Member

Choose a reason for hiding this comment

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

bye bye we won't miss you

@JammingBen
Copy link
Collaborator Author

I'll create a follow-up PR which addresses all the resolved comments, coming right up.

@JammingBen JammingBen merged commit ab5a675 into master Dec 22, 2023
2 of 3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the refactor-modal-implemetation branch December 22, 2023 13:50
@JammingBen JammingBen mentioned this pull request Dec 22, 2023
5 tasks
@JammingBen JammingBen mentioned this pull request Jan 5, 2024
@JammingBen JammingBen mentioned this pull request Jan 19, 2024
7 tasks
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.

Gap size in "copy instead" modal is not aligned with other modals Refactor modal
2 participants