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

use popover for values #7

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

rlankhorst
Copy link
Contributor

Context

The columns don't fit on a screen if options have very large values. This PR uses the new 'popover' built in browser modal to show the value in, and ads some css to style it.

Summary

This PR can be summarized in the following changelog entry:

  • Move option value to a popover.

Relevant technical choices:

  • Inspired by this: https://twitter.com/jdevalk/status/1780511519941300474, I've used the browser built in popover for the modal.
  • Style is in an scss file, not sure if this is the preferred method in this project
  • I've also excluded the assets from loading on other pages than the plugin page itself, as a suggestion.

Test instructions

  • Install a plugin with very large option values, like WooCommerce. Load the admin page in the current version: you'll see the columns won't fit on the page.
  • Install this PR, values are now replaced with buttons. On clicking them, a modal opens with the value.

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • [v ] I have tested this code to the best of my abilities.
  • I have added unit tests to verify the code works as intended.
  • I have checked that the base branch is correctly set.

Fixes #

@jdevalk
Copy link
Member

jdevalk commented Apr 19, 2024

Good idea!

Changes I've made:

  • I've improved the styling somewhat and made the content of the popover a bit less crue.
  • I've moved the inline CSS that was in the plugin into the file you created.
  • Since I don't want to add a build step for now, I'm sticking to plain CSS, since there's so little of it. So I've removed the SASS and the source map.
  • I've fixed a few minor CS issues.

I've also excluded the assets from loading on other pages than the plugin page itself, as a suggestion.

Good catch!!

@jdevalk jdevalk merged commit 4808bd8 into Emilia-Capital:main Apr 19, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants