Closed dshanske closed 4 years ago
Indents! 😉
@pfefferle I will go through and look for incorrect indents. How is it otherwise?
@pfefferle Was that the right indent?
@pfefferle Is this ready to merge?
I had not yet the time to look at the code, sorry!
@pfefferle I thought you had, misunderstood, sorry
Rebased this based on the merge of the other PR
@pfefferle Any chance to look at this one?
The settings are a bit complicated. You choose "Use Another Site as an IndieAuth Endpoint" and you have to hit save before you see the settings for the custom endpoints, you have to change them and then hit enter again. The settings are at the bottom of the page, so you might not see/recognize them on small monitors/mobile.
I would prefer to add the fields below the "Use Another Site as an IndieAuth Endpoint" option, so the user is able to edit them in the same step as he changes the option.
The settings are a bit complicated. You choose "Use Another Site as an IndieAuth Endpoint" and you have to hit save before you see the settings for the custom endpoints, you have to change them and then hit enter again.
I would prefer to add the fields below the "Use Another Site as an IndieAuth Endpoint" option, so the user is able to edit them in the same step as he changes the option.
That works. I want to see if I can get some feedback on the UI part as well.
Hide "In order to ensure IndieAuth tokens will work please visit the settings page to check: Visit Settings Page" error on the IndieAuth settings page.
Show "Authorization has Failed" description directly in the site-health check?
Show "Authorization has Failed" description directly in the site-health check?
I thought I did that?
Hide "In order to ensure IndieAuth tokens will work please visit the settings page to check: Visit Settings Page" error on the IndieAuth settings page.
That I will change. Might be to run a health check.
Show "Authorization has Failed" description directly in the site-health check?
I thought I did that?
Only the reference to the settings site with the big red header... but why not show the "howto fix that problem" directly to the health page? So we might be able to remove the big red box from the settings page some time in the future 😉
@pfefferle I moved all the error checking into the Site Health functionality. I was looking at the other question, re the fields to edit the endpoints...if you notice, those settings are not loaded until you load the remote class. I suppose I could register them regardless, then use a little JS to unhide them on the page.
I see nothing standing out as needing addressed at this point. Hesitant to approve just yet because I think @pfefferle may have a bit more first.
if you notice, those settings are not loaded until you load the remote class. I suppose I could register them regardless, then use a little JS to unhide them on the page.
I can comprehend your design decisions, but it is a new old core functionality of the plugin, so why not move the settings stuff to the settings classes/templates and only "lazy" load the functionality?
I also do not see the need of JS. I would simply load the form items under the second check box and ignore them if the first checkbox is selected.
BTW. the code renders two labels with the same for
:
<th scope="row">
<label for="indieauth_authorization_endpoint">Authorization Endpoint</label>
</th>
<td>
<label for="indieauth_authorization_endpoint">
<input id="indieauth_authorization_endpoint" name="indieauth_authorization_endpoint" type="url" value="https://indieauth.com/auth" placeholder="https://indieauth.com/auth">
</label>
</td>
@pfefferle I'd like to merge this PR. I've removed the selector for the remote endpoint for now to allow the other enhancements and refactoring to be merged. See https://chat.indieweb.org/dev/2020-07-14/1594761683886700
There are still people who want to not host their own endpoint and delegate it, but for now, want to make this an 'expert' feature.
The most recent change keeps the libraries for remote code and the enhancements to allow it in the plugin. It removes the configuration option that will enable them. This will, in a subsequent PR, become an 'expert mode' that will not be visible to users by default. Indieauth.com is no longer mentioned or used anywhere in the code.
There are several needed steps before release related to this that will be pushed in future PRs, including #168 .
This adds an HTTP/SSL check to the Site Health system, restores the remote endpoint functionality, and introduces loading logic to either allow the site to run using an external service like IndieAuth or using the built-in service without loading both simultaneously. It also turns IndieAuth_Plugin into a static class. We can't remove the class as Micropub and Microsub at this time check for its presence. The purpose of the class is to handle the load and store the object reflecting the remote/local configuration.
I tried to get the language in the settings clear about what each does.