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

Onboarding and settings refactor to React #771

Open
wants to merge 163 commits into
base: develop
Choose a base branch
from

Conversation

iamdharmesh
Copy link
Member

@iamdharmesh iamdharmesh commented May 17, 2024

Description of the Change

The PR refactors the settings and onboarding to a React-based implementation and deprecates the existing settings panel. The existing settings panel is deprecated but can still be enabled using the classifai_use_legacy_settings_panel filter.

screencapture-classifai-local-wp-admin-tools-php-2024-09-19-16_17_44

Closes #502
Closes #449

How to test the Change

Settings:

  1. Go to Tools > ClassifAI.
  2. Ensure the settings page loads correctly and that navigation works as expected.
  3. Update feature settings, such as enabling/disabling features or changing/configuring providers, and verify that everything functions as expected.
  4. Existing settings: Configure the ClassifAI plugin using the develop branch, then switch to this PR branch. Confirm that no settings are lost and that all data is correctly reflected in the new settings page.
  5. Use Legacy Settings: Add the following code to the theme’s functions.php file or plugin: add_filter( 'classifai_use_legacy_settings_panel', '__return_true' );. Verify that the old settings page loads and works as expected.

OnBoarding

  1. Deactivate the ClassifAI plugin and activate it again.
  2. You will see a notice to start the ClassifAI setup. Click the "Start Setup" button.
  3. Follow the setup process and ensure everything works as expected.
  4. Existing settings: Visit the setup page from the settings page and verify that it correctly reflects the existing configured features, with the option to modify the settings accordingly.
  5. Use Legacy Settings: Add the following code to the theme’s functions.php file or plugin: add_filter( 'classifai_use_legacy_settings_panel', '__return_true' );. Verify that the old onboarding/setup page loads and works as expected.

Add additional feature fields

  1. Add a new feature to the plugin by following the guide here.
  2. Follow step 4 to add additional feature fields to the settings and validate that everything works as expected.

Add custom Provider fields

  1. Add a custom provider to any feature by following the guide here.
  2. Add the required provider fields to the settings by following step 5 in the guide and validate that everything works as expected.

General
Run a full regression test to ensure everything is working as expected.

Changelog Entry

Added - New React-based settings and onboarding process.
Deprecated - The existing settings panel.

Credits

Props @jeffpaul @dkotter @iamdharmesh @Sidsector9

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@iamdharmesh iamdharmesh self-assigned this May 17, 2024
@github-actions github-actions bot added this to the 3.1.0 milestone May 17, 2024
@iamdharmesh iamdharmesh changed the title [WIP] Enhancement/502: Ssettings refactor to React [WIP] Enhancement/502: Settings refactor to React May 17, 2024
@Sidsector9
Copy link
Member

Sidsector9 commented May 20, 2024

@iamdharmesh below, I've checked the "Editor" checkbox under "Allowed roles". Ideally it should have just re-rendered the area pointed by the arrow, but instead the whole App re-renders. We can consider improving this.

Screenshot 2024-05-20 at 9 20 17 PM

@Sidsector9
Copy link
Member

Preventing access of state at the root App may help avoid re-rendering. I am working on and likely to fix this.

@Sidsector9
Copy link
Member

Improved areas where re-rendering wasn't necessary. I'm trying to further improve this.

Screenshot 2024-05-28 at 8 19 06 PM

@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Sep 26, 2024
@github-actions github-actions bot added needs:refresh This requires a refreshed PR to resolve. and removed needs:refresh This requires a refreshed PR to resolve. labels Sep 27, 2024
@github-actions github-actions bot added needs:refresh This requires a refreshed PR to resolve. and removed needs:refresh This requires a refreshed PR to resolve. labels Sep 27, 2024
@github-actions github-actions bot removed the needs:refresh This requires a refreshed PR to resolve. label Sep 28, 2024
} );

return (
<Fill name="ClassifAIProviderSettings">
Copy link
Member

@Sidsector9 Sidsector9 Oct 3, 2024

Choose a reason for hiding this comment

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

Note to self:

Find out a way to make this easier so that 3rd-party devs don't have to write Fill.
Maybe try exporting a component on the window object which can be used to inject custom Provider settings.

Copy link
Collaborator

@dkotter dkotter left a comment

Choose a reason for hiding this comment

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

Great work on this @iamdharmesh and @Sidsector9, this looks really nice! I've not gone through all the code line by line but at a high level this all looks good to me.

Couple thoughts:

  1. We're introducing a lot of new JS files and doesn't seem any of them have code comments in place explaining what is going on. A lot of those are self explanatory (like all the provider settings ones) but I think some of the others could benefit from either a comment at the top of the file or a comment on the individual exported methods explaining what they're used for, just making it more clear for anyone looking at the code
  2. I like how the User permissions are broken into their own section from the main settings. Could we collapse that section by default? For me it was open and I had to collapse it myself but I think a default of collapsed would be nice
  3. Curious on any discussion that was had around keeping the old settings and just hiding that behind a filter? I almost lean towards just getting rid of all that, so we're not having to maintain a bunch of legacy code
  4. @jeffpaul I think the new design looks really good but being such a major change, is this something we'd want to pull in UX resources to give a quick look at (if we haven't already)?

@iamdharmesh
Copy link
Member Author

Thank you very much for the review and for providing feedback, @dkotter.

We're introducing a lot of new JS files and doesn't seem any of them have code comments in place explaining what is going on. A lot of those are self explanatory (like all the provider settings ones) but I think some of the others could benefit from either a comment at the top of the file or a comment on the individual exported methods explaining what they're used for, just making it more clear for anyone looking at the code

Thanks for this note. Addressed in 73150b5

I like how the User permissions are broken into their own section from the main settings. Could we collapse that section by default? For me it was open and I had to collapse it myself but I think a default of collapsed would be nice

Agree. Addressed in bc39ea0

Curious on any discussion that was had around keeping the old settings and just hiding that behind a filter? I almost lean towards just getting rid of all that, so we're not having to maintain a bunch of legacy code.

I discussed this with @jeffpaul in our 1:1. This was intended to allow users who have extended or customized our old settings some extra time until we remove these settings in the upcoming major release. However, I am open to completely removing the old setup if that seems like the best path forward.

Thank you.

@jeffpaul
Copy link
Member

jeffpaul commented Nov 2, 2024

I discussed this with @jeffpaul in our 1:1. This was intended to allow users who have extended or customized our old settings some extra time until we remove these settings in the upcoming major release. However, I am open to completely removing the old setup if that seems like the best path forward.

I'm also open to jettisoning that code here to keep things easier to maintain on our end. Perhaps some note in the dev docs on migrating any extensions of the former settings?

@Sidsector9
Copy link
Member

Update

The following is the UI update as per the new design. I'll raise a separate PR.

Screenshot 2024-11-10 at 6 54 55 PM

@dkotter
Copy link
Collaborator

dkotter commented Nov 13, 2024

This was intended to allow users who have extended or customized our old settings some extra time until we remove these settings in the upcoming major release. However, I am open to completely removing the old setup if that seems like the best path forward.

I'm also open to jettisoning that code here to keep things easier to maintain on our end. Perhaps some note in the dev docs on migrating any extensions of the former settings?

I'd love to just remove it rather than having it clutter up our codebase. That said, as long as we open an issue to track the removal of this in a release or two, that's fine by me.

I do think having docs around how to migrate would be nice, though not a blocker to getting this merged in.

The following is the UI update as per the new design. I'll raise a separate PR.

@Sidsector9 Sounds like the plan is to make these updates in a separate PR, is that correct? If so, @Sidsector9, @iamdharmesh I'm fine with this getting merged in and we can iterate on things in separate PRs. Note we have a few open PRs that I think will need to be modified to work in the new settings once this gets merged in as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding and settings refactor to React Ensure all required options are in new onboarding
5 participants