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

Added admin pointer #199

Merged
merged 15 commits into from
Mar 4, 2022
Merged

Added admin pointer #199

merged 15 commits into from
Mar 4, 2022

Conversation

dainemawer
Copy link
Contributor

Summary

Fixes #193
Screenshot 2022-03-01 at 11 01 05

Relevant technical choices

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@dainemawer dainemawer added Infrastructure Issues for the overall performance plugin infrastructure Needs Review labels Mar 1, 2022
@dainemawer dainemawer added this to the 1.0.0-beta.1 milestone Mar 1, 2022
@dainemawer dainemawer added the [Type] Feature A new feature within an existing module label Mar 1, 2022
@jjgrainger jjgrainger self-requested a review March 1, 2022 14:05
Copy link
Contributor

@jjgrainger jjgrainger left a comment

Choose a reason for hiding this comment

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

Looks good @dainemawer , just a couple of minor things to address.

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@dainemawer Left a couple of comments. This looks on the right track, but there's a lot of code here that can be simplified, and overall we need to be more careful of not enqueue things too aggressively.

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
@felixarntz felixarntz changed the base branch from trunk to release/1.0.0-beta.1 March 3, 2022 00:47
@felixarntz
Copy link
Member

@dainemawer FYI Since we've branched off from trunk into a release/1.0.0-beta.1 branch, I've updated the base branch of this PR to that new branch. So going forward, please make sure to work off release/1.0.0-beta.1, and specifically not merge trunk into this again.

@dainemawer
Copy link
Contributor Author

Roger that @felixarntz will fix the merge conflict here!

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@dainemawer Left a bit more feedback here, this is looking close now.

admin/load.php Outdated Show resolved Hide resolved
admin/load.php Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
admin/load.php Outdated Show resolved Hide resolved
@mitogh mitogh requested a review from felixarntz March 4, 2022 19:05
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Awesome work @dainemawer!

Thanks @mitogh for the final iteration.

@felixarntz felixarntz dismissed jjgrainger’s stale review March 4, 2022 21:13

Feedback has been addressed.

admin/load.php Outdated Show resolved Hide resolved
Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

one tiny spacing change to align the values, otherwise 👍🏼

Co-authored-by: Adam Silverstein <[email protected]>
@mitogh mitogh merged commit fa5c12f into release/1.0.0-beta.1 Mar 4, 2022
@mitogh mitogh deleted the feature/add-admin-pointer branch March 4, 2022 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide admin pointer informing about the plugin's admin screen
5 participants