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

URLInput: Replace input with InputControl #65158

Merged
merged 17 commits into from
Sep 18, 2024
Merged

Conversation

rahulharpal1603
Copy link
Contributor

@rahulharpal1603 rahulharpal1603 commented Sep 9, 2024

What?

Fixes #64709

Why?

The URLInput component uses an input element directly, instead of a standard component like InputControl or TextControl. This will cause styles to drift from the rest of the app, since it won't receive style updates with the rest of the componentry.

Also, the LinkControl and Social-Links inputs were inconsistent, as mentioned in the issue. This is now fixed.

How?

I imported _experimentalInputControl and replaced <input> with <InputControl> also, I added the ability to pass suffix in the case of LinkControl URL Input.

LinkControl and SocialLinks are now consistent, the button outside of SocialLinks is now inside the input field:

Before:
image

After:
image

Screenshots or screencast

image

image

This PR is not ready to merge yet because of the above issues; when we try to autofill, the blue highlight is not covering the input box fully; this is due to the width not being 100%; I am not able to figure out how to fix this.

Update:

Now the above issue is resolved, it was due to CSS specificity conflicts and I have fixed that. Also the old CSS of the input tag is now cleaned up

After Fix:

Here, the highlight is not covered fully because of the suffix button being there. This one is intended behaviour:
image

In this, the highlight covers it fully:
image

Copy link

github-actions bot commented Sep 9, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: rahulharpal1603 <rahulharpal@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: jasmussen <joen@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 9, 2024
Copy link

github-actions bot commented Sep 9, 2024

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @rahulharpal1603! In case you missed it, we'd love to have you join us in our Slack community.

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

@mirka mirka requested a review from a team September 10, 2024 09:46
@mirka mirka added [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement. labels Sep 10, 2024
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

This PR is not ready to merge yet because of the above issues; when we try to autofill, the blue highlight is not covering the input box fully; this is due to the width not being 100%; I am not able to figure out how to fix this.

That's just a consequence of rendering a suffix in InputControl, which causes the underlying <input /> HTML element to be a bit narrower. I believe that the light blue background that you show in your screenshot is a browser style to highlight the input field when suggesting auto-complete values. Not sure we can do much about this, @mirka ?

packages/block-editor/src/components/url-input/index.js Outdated Show resolved Hide resolved
rahulharpal1603 and others added 3 commits September 11, 2024 21:55
skips passing a prop when suffix isn't needed.

Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Copy link
Member

@mirka mirka 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 contributing!

There is also a usage in the Social Icons block that needs to be updated to use the suffix (see discussion in original issue).

@mirka
Copy link
Member

mirka commented Sep 12, 2024

Not sure we can do much about this

Yes, that is expected behavior. Actually I'm curious when/how that autocomplete is enabled @rahulharpal1603. I've never seen it on these URL inputs, and I don't think a user would ever want it? 🤔 Maybe it's something that should be explicitly disabled.

@rahulharpal1603
Copy link
Contributor Author

Thanks for contributing!

There is also a usage in the Social Icons block that needs to be updated to use the suffix (see discussion in original issue).

But in that, the button is outside the input border and the suffix puts the button inside the border.

@mirka
Copy link
Member

mirka commented Sep 12, 2024

But in that, the button is outside the input border and the suffix puts the button inside the border.

Exactly! And we agreed that the button placement should be the same as in LinkControl #64709 (comment)

@rahulharpal1603
Copy link
Contributor Author

rahulharpal1603 commented Sep 12, 2024

But in that, the button is outside the input border and the suffix puts the button inside the border.

Exactly! And we agreed that the button placement should be the same as in LinkControl #64709 (comment)

Yes, I would make them consistent.

@rahulharpal1603
Copy link
Contributor Author

rahulharpal1603 commented Sep 15, 2024

@mirka, all of the requirements you mentioned above have been met. I have tested the final version. Please review the changes.

Also, kindly go through the updated PR description. I have attached new screenshots.

Copy link
Member

@mirka mirka 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 all the follow-ups, I really appreciate how much we got to clean up here!

@mirka mirka changed the title Replacing input with InputControl URLInput: Replace input with InputControl Sep 18, 2024
@mirka mirka enabled auto-merge (squash) September 18, 2024 13:35
@mirka mirka merged commit 48079e9 into WordPress:trunk Sep 18, 2024
61 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.3 milestone Sep 18, 2024
@rahulharpal1603
Copy link
Contributor Author

Thanks for all the follow-ups, I really appreciate how much we got to clean up here!

Thanks a lot! Your code reviews were really insightful.

@rahulharpal1603
Copy link
Contributor Author

rahulharpal1603 commented Sep 18, 2024

Thanks for all the follow-ups, I really appreciate how much we got to clean up here!

Also, I am curious about one more thing. When would the changes made by this code reflect on the wordpress website?

@mirka
Copy link
Member

mirka commented Sep 18, 2024

If your site is using the Gutenberg beta plugin (released every two weeks), then it'll be in the next 19.3 release. If not, you'll see it in the next major WordPress release, which is 6.7.

</InputControlSuffixWrapper>
)
}
props
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this props here which looks like a typo — should we remove it? Was it meant to be something like `{...props} ? @mirka

Copy link
Contributor Author

@rahulharpal1603 rahulharpal1603 Sep 24, 2024

Choose a reason for hiding this comment

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

Yes, I think that this is a typo; "props" was never there in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

Cleanup in #65650

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URLInput should use InputControl internally
4 participants