activist icon indicating copy to clipboard operation
activist copied to clipboard

923 - Modal still open ...

Open mattburnett-repo opened this issue 1 year ago • 5 comments

Contributor checklist


Description

This PR addresses issue 923. The changes affected modal components that use an 'isOpen' prop; the 'isOpen' props and related functionality were removed.

Modals related to Organizations were affected. Other entities (ie Events) were not checked.

Testing was done following the original description of the problem in issue 923 (click activist icon, use forward / backward arrows to navigate, etc.). However, there appears to be an error with reloading the organization data when clicking the back button to check if modal is not displayed, so we can't be completely sure that everything is fixed.

That being said, the affected modals appear to work correctly without the 'isOpen' props, meaning that they appear to operate normally.

Related issue

  • #923

mattburnett-repo avatar Aug 04 '24 15:08 mattburnett-repo

Thank you for the pull request!

The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • [x] The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo
  • [x] The TypeScript and formatting workflows within the PR checks do not indicate new errors in the files changed

  • [x] The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • [x] The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

github-actions[bot] avatar Aug 04 '24 16:08 github-actions[bot]

Deploy Preview for activist-org ready!

Name Link
Latest commit 9cbe8d84eb944afef7a4e4fa24b7a3466358d4d6
Latest deploy log https://app.netlify.com/sites/activist-org/deploys/66b8f5059220c600083bcb24
Deploy Preview https://deploy-preview-941--activist-org.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Aug 04 '24 16:08 netlify[bot]

One question on this, @mattburnett-repo: What the purpose of all the Watch for closeModal emit and do cleanup when it happens. in the codebase now? Can we remove these somehow? And can we clean up the command palette code a bit to remove the TODOs from it? Maybe you could let me know what issues could be made from those TODOs, and then I can make the issues and we can remove the comments?

andrewtavis avatar Aug 11 '24 13:08 andrewtavis

The 'Watch for closeModal emit...' are there just in case they are needed in the future. If you prefer, they can be removed.

The TODO messages in the command palette are notes-to-self. Later this evening I will go through them and let you know which of these are issue-relevant.

mattburnett-repo avatar Aug 11 '24 14:08 mattburnett-repo

Here's a list of issues from the TODO's removed from ModalCommandPalette.vue

Search works, but it's picking up the literal values of 'displayName' (eg. "_global.home"). Search should instead look through the rendered values.

Sometimes router.push() loses the i18n part of the path (eg: /en/the.path.name)

mattburnett-repo avatar Aug 11 '24 16:08 mattburnett-repo