-
Notifications
You must be signed in to change notification settings - Fork 130
Add an indicator in the adminbar to show when using SQLite #604
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
Conversation
@aristath The change looks good to me, but I think we need to evaluate whether this is generally considered useful enough to take a spot in the admin bar. I think it's a bit too late for 1.8.0 since it's an enhancement that was just opened today and needs a bit of thought, so will move this to 1.9.0. |
@aristath We still haven't had any conversation about this. For example here's some questions I have which would be good to discuss:
Last but not least, should we do this or should we not do this? @bethanylang This is probably not a massive conversation to reserve an entire chat for, but still it could be a good topic to cover in one of the next ones. For now I'll move this to the next release (2.0.0) since we need to create the 1.9.0 release branch today. |
@aristath Wondering what your take is on the above? What led you to opening this PR? :) |
Initially, when someone was activating the module, they were getting the install screen where they would set their site-name etc. With that workflow, I was just setting the site-name to |
Thanks @aristath, makes sense. I was just a bit concerned to use the limited WP Admin bar space, but that use-case is reasonable to me. However, I would advise we also show that menu item when not using SQLite, just as long as the module is active. We also need to consider the value of the
Or alternatively, we could just rely on Let me know what you think. My main feedback would be to show the menu even when SQLite is not enabled but the module is enabled, as that would be a good indicator that something is wrong. |
@aristath Friendly ping ^ |
Sounds good to me 👍 |
5c49aca
to
5dfa9c2
Compare
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aristath A bit of additional follow up feedback.
modules/database/sqlite/load.php
Outdated
/* translators: %s: SQLite icon. */ | ||
'title' => defined( 'PERFLAB_SQLITE_DB_DROPIN_VERSION' ) && defined( 'DATABASE_TYPE' ) && 'sqlite' === DATABASE_TYPE | ||
? '<span style="color:#46B450;">' . __( 'Database: SQLite', 'performance-lab' ) . '</span>' | ||
: '<span style="color:#DC3232;">' . __( 'Database: MySQL', 'performance-lab' ) . '</span>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment #604 (comment) (also #604 (comment)), we should use $wpdb->db_server_info()
to display more accurate information. It's not always MySQL, it can also be MariaDB.
Also noting that inlining this conditional here makes this hard to parse, we can simplify this by e.g. using a $title
variable.
modules/database/sqlite/load.php
Outdated
@@ -58,3 +58,23 @@ function perflab_sqlite_plugin_admin_notice() { | |||
); | |||
} | |||
add_action( 'admin_notices', 'perflab_sqlite_plugin_admin_notice' ); // Add the admin notices. | |||
|
|||
/** | |||
* Add a link to the admin bar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function descriptions should start with a third-person verb per WP docs standards. Also, we need to add a @since
annotation.
* Add a link to the admin bar. | |
* Adds a link to the admin bar. | |
* | |
* @since n.e.x.t |
@aristath In the interest of time, I've quickly addressed the outstanding feedback myself in 6bf9328, since we preferably need to merge this today to get it into the 2.0.0 release. @SergeyBiryukov Can you give this a quick review, since you previously commented (and I rather don't just approve my own additions 😄)? |
@aristath Can you add this change to the standalone plugin as well please? |
Will do 👍 |
I'm gonna go ahead and merge this in order to create the new release branch. If there's anything that should be changed, we can always open a quick follow up PR against that branch |
Summary
Adds an indicator in the adminbar to show when using SQLite:
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.