-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Fix URL input fields to reserve space for spinner icon and prevent text overlap #70134
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
base: trunk
Are you sure you want to change the base?
Fix URL input fields to reserve space for spinner icon and prevent text overlap #70134
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
Thanks for the PR!
When defining styles, it's important to note the following:
- Don't use selectors with the
components-
prefix as much as possbile: the internal implementation of the component may change. - Don't use hard-coded values for spacing. Use 8px-based
$grid-unit-xx
. - Avoid using !important as much as possible.
I hope you'll explore approaches that meet these requirements. Additionally, right padding should not be necessary when the spinner is not visible.
@t-hamano I experimented with this, and I noticed that when the spinner appears, the text either shifts to the left or gets overlapped by the spinner, which doesn’t feel quite right. I’d recommend reserving space for the spinner to avoid this issue. Also, Its not possible to avoid |
This is understandable, but it affects every place the For example, the Social Icon block has space to the right even though the spinner is not visible at all: I would suggest the following changes to the trunk. What do you think? diff --git a/packages/block-editor/src/components/url-input/style.scss b/packages/block-editor/src/components/url-input/style.scss
index 579b311766..3bc8e68c37 100644
--- a/packages/block-editor/src/components/url-input/style.scss
+++ b/packages/block-editor/src/components/url-input/style.scss
@@ -28,6 +28,10 @@ $input-size: 300px;
top: calc(50% - #{$spinner-size} / 2);
right: $grid-unit;
}
+
+ &:has(.components-spinner) .components-input-control__input {
+ padding-right: $grid-unit-40;
+ }
}
// Suggestions |
Excellent idea! lets go ahead with your way. |
FYI this approach also gives that bouncy effect when the spinner appears & the text is overlapped by the spinner too. |
I understand this. At the moment, there doesn't seem to be an ideal solution if we want to keep the spinner. I'd also like to hear from people who have worked on this component in the past. cc @getdave @scruffian @WordPress/gutenberg-design ProblemWe are trying to solve the overlap between the text and the spinner. First approach (current state of this PR)Renders right padding conditionally. Disadvantages: Causes text bouncing each time the spinner changes visibility. 6d9be2dd04b354b54ed62830f083526d.mp4Second approachAlways reserve the right padding for the spinner. Disadvantages: Right padding is always applied, even if we don't render a spinner at all. |
In a sense, the spinner is there to indicate "busy", that the input field is in a state where you have to wait before continuing. Does it make sense in this case to change the line-break indicator to the spinner-state, while this is happening? Since focus could be there we probably can't replace it outright, but potentially that button could just be set to isBusy while working. |
@jasmussen Thank you for the great feedback! @sarthaknagoshe2002 Would you like to try the proposed approach? |
I had the same alternative in my mind, I'll re-work on this soon. |
I’ve now set the button to isBusy when the user types, and completely removed the spinner. Let me know if there’s a better way to handle this. |
I missed an important point. Because the Let's consider a different approach. |
Maybe we could add a check to ensure that |
I don't think this approach is appropriate. Because if a component that doesn't meet this condition is used as a suffix, there will be no element to represent the loading state, and the user will have no way of knowing if it's loading or not. Of the two remaining approaches, I tend to prefer the former:
In the future, it may be necessary to completely rethink the UI itself.. Quoting from this comment:
|
Another approach could be to always reserve the right padding for the spinner when there is a suffix. This will ensure that the padding is reserved only for those url-inputs where a suffix is set. |
I don't think there's any relation between providing the My understanding is that the spinner may be rendered when both Detailsdiff --git a/packages/block-editor/src/components/url-input/index.js b/packages/block-editor/src/components/url-input/index.js
index 60cbf4d5e8..bfed542739 100644
--- a/packages/block-editor/src/components/url-input/index.js
+++ b/packages/block-editor/src/components/url-input/index.js
@@ -422,6 +422,8 @@ class URLInput extends Component {
instanceId,
placeholder = __( 'Paste URL or type to search' ),
__experimentalRenderControl: renderControl,
+ __experimentalShowInitialSuggestions: showInitialSuggestions,
+ __experimentalHandleURLSuggestions: handleURLSuggestions,
value = '',
hideLabelFromVision = false,
} = this.props;
@@ -441,6 +443,7 @@ class URLInput extends Component {
label,
className: clsx( 'block-editor-url-input', className, {
'is-full-width': isFullWidth,
+ 'has-spinner': showInitialSuggestions && handleURLSuggestions,
} ),
hideLabelFromVision,
};
diff --git a/packages/block-editor/src/components/url-input/style.scss b/packages/block-editor/src/components/url-input/style.scss
index 579b311766..3224fcaf06 100644
--- a/packages/block-editor/src/components/url-input/style.scss
+++ b/packages/block-editor/src/components/url-input/style.scss
@@ -22,6 +22,11 @@ $input-size: 300px;
}
}
+
+ &.has-spinner .components-input-control__input {
+ padding-right: $grid-unit-40;
+ }
+
.components-spinner {
position: absolute;
margin: 0; This ensures that navigation links and inline links have padding for the spinner, but the social icon block does not have padding. |
This methods seems to solve all our issues. I think this is ready to be merged! Thanks @t-hamano! |
Sorry, I think the conditional statements I suggested were incorrect. In some cases, the right padding is not applied as expected. In all of the following scenarios, right padding must be applied appropriately: ❌ Button block: Input URL from the block toolbarEven though the spinner is visible, it has no right padding. ✅ Format Library: Inline link✅ Social Link block: Input URL❌ Image block: Input URL from the block toolbarEven though the spinner is visible, it has no right padding. ✅ Image block: Change the current media URL: |
For me for some reason the media works fine! I'll need to check for the button block. |
What?
This PR addresses a UI issue where the spinner icon in URL input fields overlaps with and hides long user-entered text.
Closes #58679
Why?
URL input field do not reserve space for the spinner icon. As a result, during loading states, long URLs are partially hidden, which disrupts user experience and readability.
How?
A simple padding was needed towards the left of the spinner
Testing Instructions
Screenshots or screencast