-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
@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. |
Preventing access of state at the root App may help avoid re-rendering. I am working on and likely to fix this. |
…i into enhancement/react-settings
08e103f
to
b0bd72b
Compare
} ); | ||
|
||
return ( | ||
<Fill name="ClassifAIProviderSettings"> |
There was a problem hiding this comment.
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.
There was a problem hiding this 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:
- 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
- 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 - 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
- @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)?
Thank you very much for the review and for providing feedback, @dkotter.
Thanks for this note. Addressed in 73150b5
Agree. Addressed in bc39ea0
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. |
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.
@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. |
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.Closes #502
Closes #449
How to test the Change
Settings:
Tools
>ClassifAI
.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.add_filter( 'classifai_use_legacy_settings_panel', '__return_true' );
. Verify that the old settings page loads and works as expected.OnBoarding
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
Add custom Provider fields
General
Run a full regression test to ensure everything is working as expected.
Changelog Entry
Credits
Props @jeffpaul @dkotter @iamdharmesh @Sidsector9
Checklist: