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

feat: support "Show Toolbar when viewing site" user setting #2763

Merged
merged 11 commits into from
Apr 12, 2023
Merged

feat: support "Show Toolbar when viewing site" user setting #2763

merged 11 commits into from
Apr 12, 2023

Conversation

blakewilson
Copy link
Contributor

@blakewilson blakewilson commented Mar 15, 2023

What does this implement/fix? Explain your changes.

This PR adds the ability to query for the "Show Toolbar when viewing site" user preference in WPGraphQL. This is particulary helpful if you have created an "admin bar" alternative on your Headless site and want it to be displayed based on this preference.

isToolbarVisible.mov

Does this close any currently open issues?

#2764

Any relevant logs, error output, GraphiQL screenshots, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Any other comments?

Where has this been tested?

Operating System: MacOS Ventura 13.1

WordPress Version: 6.1.1

Unit tests have been added.

src/Model/User.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

I agree this data should be exposed in core and the PR itself looks good to me (see feedback on field name).

I do worry that exposing a single user meta field without a greater strategy (e.g. #479) is going to lead to more code debt and schema breaks/deprecations, but I'm not anticipating anything specific, just recommending this be reviewed in the larger context.

src/Model/User.php Outdated Show resolved Hide resolved
@justlevine justlevine added type: enhancement Improvements to existing functionality status: in review Awaiting review before merging or closing object type: user Relating to the User Type component: query Relating to GraphQL Queries labels Mar 15, 2023
@coveralls
Copy link

coveralls commented Mar 15, 2023

Coverage Status

Coverage: 85.12% (+0.009%) from 85.112% when pulling 9835f7a on blakewilson:support-viewer-toolbar-visibility into 44fbc68 on wp-graphql:develop.

blakewilson and others added 3 commits March 16, 2023 09:57
Co-authored-by: Joe Fusco <josephfusco@users.noreply.github.com>
Co-authored-by: Dovid Levine <justlevine@gmail.com>
@blakewilson
Copy link
Contributor Author

I agree this data should be exposed in core and the PR itself looks good to me (see feedback on field name).

I do worry that exposing a single user meta field without a greater strategy (e.g. #479) is going to lead to more code debt and schema breaks/deprecations, but I'm not anticipating anything specific, just recommending this be reviewed in the larger context.

I agree with you here. Is there anything you see in this PR that can address these worries?

@justlevine
Copy link
Collaborator

Is there anything you see in this PR that can address these worries?

@blakewilson the PR actually looks great to me, my comment was more a suggestion for @jasonbahl to do a basic gut-check before merging than anything else.

From memory (beyond skimming #479, I havnt looked through other tickets) the things I know we should keep in mind are:

  • the auth layer (the field is private here ✔️)
  • the field name makes semantic sense wherever it's exposed (✔️✔️)
  • the default meta value ( that's good here too, but we should double check we don't need to loose-comparison to handle '0', 1 or other weird WP meta quirks around booleans)

composer.json Outdated Show resolved Hide resolved
'shouldShowAdminToolbar' => function () {
$toolbar_preference_meta = get_user_meta( $this->data->ID, 'show_admin_bar_front', true );

return 'true' === $toolbar_preference_meta ? true : false;
Copy link

Choose a reason for hiding this comment

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

Avoid too many return statements within this method.

@codeclimate
Copy link

codeclimate bot commented Mar 21, 2023

Code Climate has analyzed commit 9835f7a and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@blakewilson
Copy link
Contributor Author

blakewilson commented Mar 22, 2023

@justlevine Could you help initiate the other workflows? 🙏 Looks like linting is passing now.

@jasonbahl
Copy link
Collaborator

Ultimately, I'd like this to be resolved more centrally where register_user_meta could map to the WPGraphQL Schema.

That's a bigger initiative and since this has a test, we can ensure if we do tackle that initiative in the future, this continues to work as-is.

@jasonbahl jasonbahl mentioned this pull request Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: query Relating to GraphQL Queries object type: user Relating to the User Type status: in review Awaiting review before merging or closing type: enhancement Improvements to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants