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

Link Control - Add support for text only labels #47930

Merged
merged 6 commits into from
Feb 22, 2023

Conversation

abhi3315
Copy link
Contributor

@abhi3315 abhi3315 commented Feb 9, 2023

What?

Shows the text labels only in the Link Control popover.

Why?

#47545
When the Show button text labels toggle is enabled from the Gutenberg preferences, LinkControl is only showing the setting icon. This PR will display the text label or icon in LinkControl according to preferences.

How?

  • Getting the showIconLabels from the preferences data store.
  • Add the show-icon-labels CSS class to the component accordingly to hide or display the icon/text.

Testing Instructions

  • Open the Gutenberg preferences and toggle the Show button text labels.
  • Check if the icon and text labels are showing accordingly in the LinkControl.

Testing Instructions for Keyboard

Screenshots or screencast

image

@abhi3315 abhi3315 requested a review from getdave as a code owner February 9, 2023 13:25
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 9, 2023
@github-actions
Copy link

github-actions bot commented Feb 9, 2023

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @abhi3315! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@abhi3315 abhi3315 changed the title feat(link-control): show text only labels Link Control - Add support for text only labels Feb 9, 2023
@getdave getdave added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [a11y] Labelling labels Feb 9, 2023
@getdave
Copy link
Contributor

getdave commented Feb 9, 2023

@abhi3315 Thank you for the PR. I have looped in @tellthemachines who has implemented labels like this before and may have a view.

I think it's safe to shorten the toggle text to just Link Settings as the aria- attributes probably do enough to communicate the function of the control.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! I left some suggestions below.

const showIconLabels = useSelect(
( select ) =>
select( preferencesStore ).get(
'core/edit-post',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a block editor component, so it gets used across both post and site editors. Unfortunately the text label settings are specific to each editor, so using edit-post preferences here means that if the preference is set differently in the site editor, this component won't reflect it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great spot @tellthemachines. Some more context on @abhi3315. Basically the block editor package is designed to be used standalone outside of "WordPress". Therefore we can't query anything like core store or even preferences namespaces.

To work around this we often create editor "settings" which we:

  • set in the editor instances (post, widgets and site) - passing through to the block editor component
  • consume within the block editor package (e.g. here in Link Control) to access the data

And example of this can be found in

packages/edit-site/src/components/block-editor/index.js

If you do it like this you could have a single setting for the text labels passed into the block editor. Then you could consume that value.

Does that sound suitable @tellthemachines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @getdave @tellthemachines for explaining to me thoroughly. 🙇🏻‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case we don't need to create an editor setting, because the classname we hook the styles onto is being applied to the interface component in both the post and the site editor, so we can make this change just by adding some CSS ✨

@@ -283,7 +294,9 @@ function LinkControl( {
<div
tabIndex={ -1 }
ref={ wrapperNode }
className="block-editor-link-control"
className={ classnames( 'block-editor-link-control', {
Copy link
Contributor

Choose a reason for hiding this comment

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

The classname doesn't need to be added here, because it's already added at the top level of the interface skeleton.

Instead, I'd suggest re-structuring the CSS below so it takes advantage of that top-level classname, similarly to the block toolbar CSS.

@abhi3315 abhi3315 requested review from tellthemachines and getdave and removed request for ntwb, ajitbohra, nerrad, tellthemachines and getdave February 13, 2023 10:05
@abhi3315
Copy link
Contributor Author

Thanks, @tellthemachines for the feedback.
I have made the changes. Please review it again.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the update, it's working well now! Just a little niggle about the CSS structure, but otherwise looks OK!

@@ -22,6 +23,22 @@ $preview-image-height: 140px;
width: 90vw;
max-width: $modal-min-width;
}

.show-icon-labels & {
#{$root}__tools {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need it to be this specific, it's fine to nest .components-button.has-icon directly under .show-icon-labels &. The decreased specificity also has the advantage that if we ever add another icon button to the link control, text label should just work for it out of the box 🙂

@abhi3315
Copy link
Contributor Author

abhi3315 commented Feb 14, 2023

@tellthemachines I have made the changes. Please review it again.

@getdave
Copy link
Contributor

getdave commented Feb 21, 2023

The way I do this is.

  • checkout trunk and update it.
  • switch to my branch
  • git rebase trunk
  • resolve conflicts (you may need git rebase --continue
  • you should end up with a branch with your changes played on top of the changes in trunk.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

In this shape the code of the PR is great. Tested and works 🚢 it

@getdave
Copy link
Contributor

getdave commented Feb 22, 2023

Re-running the failed e2e tests here. I'd like to be sure that we didn't introduce any regressions especially as the failures were regarding modal focus management. There's an outside chance that adding the slot may have caused an unexcepted knock on effect.

Let's see. Would also be good to get a 👍 on this from @tellthemachines.

@getdave
Copy link
Contributor

getdave commented Feb 22, 2023

Looks like test failures should be fixed by #48236. Let's rebase against trunk again to bring this one in and see if it fixes the test for us.

@abhi3315 abhi3315 force-pushed the feat/link-control-show-labels-only branch from 065eae1 to 9f2673e Compare February 22, 2023 14:57
@abhi3315
Copy link
Contributor Author

@getdave I have rebased the branch.

@abhi3315
Copy link
Contributor Author

@getdave All the tests are cleared. 🎉

@getdave getdave merged commit 2f294e1 into WordPress:trunk Feb 22, 2023
@github-actions
Copy link

Congratulations on your first merged pull request, @abhi3315! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 15.3 milestone Feb 22, 2023
@getdave
Copy link
Contributor

getdave commented Feb 22, 2023

Congrats on this PR @abhi3315 👏 You can check trunk now to see it available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs User Documentation Needs new user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants