a8cteam51 / feedland-blogroll

GNU General Public License v2.0
3 stars 0 forks source link

Update settings page #9

Closed fmfernandes closed 5 months ago

fmfernandes commented 5 months ago

Changes proposed

This PR addresses #8. On the settings page (/wp-admin/options-general.php?page=feedland_blogroll_settings) more options were added:

There's a small description for username and placeholders for helping.

Before and after screenshots

Before After
Screenshot 2024-04-10 at 00 52 23 Screenshot 2024-04-10 at 00 50 40

[!NOTE]
After screenshot is outdated. As per the latest couple of commits, there's no more an input for server and category. See Nick's reply: https://github.com/a8cteam51/feedland-blogroll/pull/9#issuecomment-2052081200

Testing

Known issues

There's no input validation on the plugin side, i.e. we do not validate if a given server URL is in fact a FeedLand server or if the category/username exist.

A possible solution for this is:

Or:

We could implement those checks on the plugin side by checking an endpoint first for user existence and/or category and render a message in the shortcode if not properly configured.

Screenshot of the rendered shortcode when improperly configured

Screenshot 2024-04-10 at 00 55 38 _This is using the Twenty Twenty-Three theme._


[!IMPORTANT]
This PR implements no mechanism for updating the default options added after a version is released, so it assumes you'll deactivate an reactivate the plugin so the new options are present.

NickGreen commented 5 months ago

We could implement those checks on the plugin side by checking an endpoint first for user existence and/or category and render a message in the shortcode if not properly configured.

I think this is the safest route, from the plugin's perspective, since it would also validate against FeedLand connection errors.

Could we just verify that an OPML file is being returned from the OPML URL, and add an admin notice if nothing is there?


Note: in the https://github.com/a8cteam51/feedland-blogroll/issues/8#issuecomment-2048059740, it is being discussed whether FeedLand.com should be the default server, so I would expect that to be a further change in this PR.

fmfernandes commented 5 months ago

Hey @NickGreen, can you please check the latest commit (8f49a78)? Specially the feedland_validate_options function which handles validation on our side upon saving the settings.

I'm not sure if it would be best to split each setting into its own but I'm happy moving forward with an array. I'm thinking about that because I think the sanitize_callback function would be better applied to each setting individually but since we do not need any other validation I think we're good moving forward with that implementation. Let me know what you think.


I'm using the /isuserindatabase?screenname={username} endpoint to check if the user exists and then trying to grab the feed from /getfeedlistfromopml?url={url} to get the feed. FeedLand will throw the message I mentioned in the PR if it can't be found, thus, defaulting to '' for the category.

NickGreen commented 5 months ago

@fmfernandes This worked great for me! Excellent work. I didn't even have to deactivate/reactivate the plugin, it just worked.

A couple of changes:

  1. The default URL needs to be https://feedland.com (instead of .social)
  2. I found the input placeholders to be distracting. For example, when you leave the category blank, it still kind of looks like the category is blogroll. My suggestion would be to just leave the placeholder text out of the inputs.
  3. How can we indicate that the category is optional? It seems like it needs to be filled in, just from looking at it right now, so maybe we can just add Category (optional) or something simple like that?

That's it, though, the code looks good and adheres to WP standards, is properly escaped, etc.

scripting commented 5 months ago

Don’t make the category optional, make it default to blogroll, for the reason you mention.

--

fmfernandes commented 5 months ago

@NickGreen, agreed on the placeholder! Removed it and also changed the server.

It's worth noting, though, that @scripting doesn't have a blogroll category set up on feedland.com, so the validation fails and we default to a category that doesn't exist.

The plugin checks the https://feedland.com/getfeedlistfromopml?url=https%3A%2F%2Ffeedland.com%2Fopml%3Fscreenname%3Ddavewiner%26catname%3Dblogroll URL which contains the message we look for the error and still uses it.

If we default it to optional, then the URL would be https://feedland.com/getfeedlistfromopml?url=https%3A%2F%2Ffeedland.com%2Fopml%3Fscreenname%3Ddavewiner which returns a valid response.

NickGreen commented 5 months ago

@fmfernandes As per the Slack convo this morning, the plan is to leave category out entirely. So, the category input goes away, and we don't build a cat modifier into the URL string.

scripting commented 5 months ago

@NickGreen -- do you agree with the idea? i could be wrong. you're the one with the eyes on the actual software here. i'm not watching very closely, here to advise.

NickGreen commented 5 months ago

@scripting Totally on board with simplifying the UI in this way. We can always "pave the cowpaths" later, right?

fmfernandes commented 5 months ago

@NickGreen, implemented changes on 66cd60a.

NickGreen commented 5 months ago

LGTM! This is good to merge.

NickGreen commented 5 months ago

Screenshot in current state for posterity: SCR-20240412-issb