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

Update settings on template activation #2144

Conversation

doekenorg
Copy link
Contributor

@doekenorg doekenorg commented Sep 17, 2024

This PR addresses #2117

After activating or installing a plugin, the selectors are updated; and the settings block is re-rendered and replaced. This will ensure all the new settings are available.

💾 Build file (12afca1).

@mrcasual
Copy link
Collaborator

@doekenorg, is this PR ready for review/testing? If so, please update the Project information, @ tag folks, etc.

@doekenorg doekenorg assigned doekenorg and Mwalek and unassigned doekenorg Sep 25, 2024
@doekenorg
Copy link
Contributor Author

I updated the connected issue, but I have now updated this one as well. I'm not sure which I should update now. Doing both sounds too much.

Copy link
Member

@zackkatz zackkatz left a comment

Choose a reason for hiding this comment

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

Looks good code-wise @doekenorg 👍 Just a couple commented-out lines.

assets/js/admin-views.js Outdated Show resolved Hide resolved
assets/js/admin-views.js Outdated Show resolved Hide resolved
@mrcasual
Copy link
Collaborator

mrcasual commented Sep 26, 2024

@Mwalek, please test. Make sure that everything is synchronized across 3 places where a layout can be selected.

@mrcasual mrcasual added this to the 2.29 milestone Sep 26, 2024
@Mwalek
Copy link

Mwalek commented Sep 26, 2024

@doekenorg @mrcasual looks good! I tested with PHP versions 7.4 & 8.3.

While testing this I found a few minor issues that are indirectly related to this functionality. @mrcasual before I create GH issues for them, could you please have a look and see if they're all things we want to fix?

Loom Video: https://www.loom.com/share/0665547e6d8b4a7dbb500ef8fb0bb618

  • In the default template selection layout, aside from the outline on the button, there is no indication that the plugin is installing.
  • Clicking "Install and Activate" or "Activate" from the placeholder behaves differently than performing the same action from the dropdown. The page is reloaded and data is lost.
  • Placeholders in the settings menu show the "Upgrade" option when hovered, even though the All Access license is active. It should display "Install/Activate" instead.
  • When using the placeholder to activate, the user is prompted twice for confirmation about leaving the page: once via the page itself and once through a browser modal.

@mrcasual
Copy link
Collaborator

mrcasual commented Sep 26, 2024

@Mwalek, the status indicator, upgrade badge, and confirmation messages are minor issues that should be corrected. What's more concerning is that when you click to install and use DataTables, the View type dropdown still shows Table.

In the order of priority:

  • 1) When creating the View and selecting the View type, the View type of the Multiple Entries layout should be the same.
  • 2) Loading indicator should be displayed when installing the layout in the View type selection during View creation
  • 3) Upgrade badge + confirmation message (these can also be resolved under a separate PR)

@doekenorg, over to you.

@mrcasual mrcasual assigned doekenorg and unassigned Mwalek Sep 26, 2024
@mrcasual mrcasual removed this from the 2.29 milestone Sep 26, 2024
@doekenorg
Copy link
Contributor Author

I've created #2158 to address the other issues.

@doekenorg
Copy link
Contributor Author

@Mwalek Back to you for (hopefully final) testing.

@doekenorg doekenorg assigned Mwalek and unassigned doekenorg and Mwalek Oct 1, 2024
@Mwalek
Copy link

Mwalek commented Oct 1, 2024

@doekenorg thank you! I retested this and can confirm that the following issues are resolved by the latest build:

  1. When creating the View and selecting the View type, the View type of the Multiple Entries layout should be the same.
  2. Loading indicator should be displayed when installing the layout in the View type selection during View creation

@doekenorg doekenorg assigned mrcasual and unassigned Mwalek Oct 1, 2024
@mrcasual mrcasual merged commit b750f7c into develop Oct 1, 2024
1 check failed
@mrcasual mrcasual deleted the issue/2117-state-not-updated-across-page-elements-after-installation-or-activation branch October 1, 2024 21:52
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.

4 participants