Skip to content

Reorganizes post options in Settings. - #4767

Merged
kurzee merged 3 commits into
developfrom
issue/settings-device-section
Feb 4, 2016
Merged

Reorganizes post options in Settings.#4767
kurzee merged 3 commits into
developfrom
issue/settings-device-section

Conversation

@kurzee

@kurzee kurzee commented Feb 4, 2016

Copy link
Copy Markdown
Contributor

Resolves the conversation and issues raised in #4756, #4757 and #4766.

After much thought and conversation with @aerych. The best approach moving forward allows for both admin and non-admin users of XML-RPC sites to toggle default post options under a new section in settings titled "This Device".

This PR also moves "Geotagging" under "This Device" for all users, XML-RPC and REST.

REST users only have default post options available if they are admins of the connected site and under the section "Writing". Non-admins will instead default to what the admin has selected for the site. (This is in parity with Calypso on the web)

REST Testing:

  1. Open Site Settings for a site connected via the REST-API and as an admin user.
  2. Observe the available default category, post format and related posts options under "Writing".
  3. Observe the available Geotagging option under "This Device".
  4. Selecting any of the available options updates both in-app and on the site.

REST Testing:

  1. Open Site Settings for a site connected via the REST-API and as a non-admin user.
  2. Observe no available "Writing" section.
  3. Observe the available Geotagging option under "This Device".

XML-RPC Testing:

  1. Open Site Settings for a site connected via the XML-RPC as admin or non-admin user.
  2. Observe the availability for the default category, post format options under "This Device".
  3. Observe the available Geotagging option under "This Device" as well.

@aerych please review and @mattmiklic on the text review, one more time. 😃Thanks so much for your input guys.

@kurzee kurzee added this to the 6.0 milestone Feb 4, 2016
@mattmiklic

Copy link
Copy Markdown
Member

Tested again, everything's looking good to me.

[self configureDefaultPostFormatCell];
return self.defaultPostFormatCell;
}
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is nitpicky code style stuff, but for consistency within the file maybe remove the parenthesis in the case statements and remove the breaks since each case returns?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The style consistency of the switch cases and breaks in this file are all over the place. I'm going to leave this alone for now as it would require touching a lot more code than the context of this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fair enough :)

@aerych

aerych commented Feb 4, 2016

Copy link
Copy Markdown
Contributor

Hiya @kurzee :)

I left some super nitpicky code style comments. I tested settings on wpcom blogs with both admin and non-admin users, and a self-hosted test blog via XML-RPC. Everything worked as described. (Yay!)

I did notice an edge case that is probably not a blocker (existing issue I think) but I want to make a note of it here. Steps to reproduce:

  • Start with a fresh install and sign into a self-hosted blog that is connected to Jetpack.
  • Go to settings and confirm that there are options under This Device for geotagging, default post format and default category.
  • Go to the Me tab and sign into the wpcom account that Jetpack is connected to.
  • Switch back to the My Sites tab. It should still be showing the settings screen, however the options on the screen have not updated to reflect a site with a REST API connection. The options for default post format and default category are missing from the Writing section and the Remove Site option is still visible.
  • Tap the back button to return to the site detail.
  • Tap to view settings again. Everything is fixed and appears as it should.

I recommend we open an new issue for the wonky behavior rather than draw out this patch any further. :)

Once the code style comments are addressed I say :shipit:! Nice work yo!

@kurzee

kurzee commented Feb 4, 2016

Copy link
Copy Markdown
Contributor Author

Merging. Thanks @aerych! Will create a separate issue for the edge case mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants