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

chore: add semicolon to textarea autocomplete attribute #9389

Merged
merged 3 commits into from
Jun 12, 2023

Conversation

kaf-lamed-beyt
Copy link
Contributor

@kaf-lamed-beyt kaf-lamed-beyt commented Jun 6, 2023

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
  • MDN issue is filed: …

(See WHATWG Working Mode: Changes for more details.)


/indices.html ( diff )

@keithamus
Copy link
Contributor

This doesn't look to be in the right place, is it?

@keithamus keithamus added the editorial Changes that do not affect how the standard is understood. label Jun 6, 2023
@kaf-lamed-beyt
Copy link
Contributor Author

This doesn't look to be in the right place, is it?

I think it is.

From the issue's description, it says autocomplete is missing the ; before cols

source Outdated
@@ -130515,7 +130515,7 @@ interface <dfn interface>External</dfn> {
<td><span data-x="text content">text</span></td>
<td><span data-x="global attributes">globals</span>;
<code data-x="attr-fe-autocomplete">autocomplete</code>
Copy link

Choose a reason for hiding this comment

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

@keithamus means it’s this line that should end with the semicolon (i.e., it should look the same as the rest of them). Line breaks are whitespace in HTML, so prefixing the next line introduces a space in the rendered text that shouldn’t be present.

screenshot of rendered spec with extra whitespace between “autocomplete” and the semicolon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Thanks for clarifying. I'll have that fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change.

cc: @keithamus @bathos

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure the latest commit has improved matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Maybe I should just close this PR and open another one.

Would that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you were on the right track with the original commit. You can try to rebase this branch to get back to that commit, or set up a new PR and close this one out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! I'll try rebasing.

Thanks for the guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keithamus kindly take a look now

Copy link
Contributor

Choose a reason for hiding this comment

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

Really close but there are now 2 semicolons when I think there should be one. I’ve added a suggestion which if accepted I think will result in the diff we want.

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 for pointing this out. I've accepted the suggestion.

<code data-x="attr-fe-autocomplete">autocomplete</code>
<code data-x="attr-textarea-cols">cols</code>;
<code data-x="attr-fe-autocomplete">autocomplete</code>;
; <code data-x="attr-textarea-cols">cols</code>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried accepting the change. But, whenever I try committing, I get this 👇🏼

image

@keithamus
Copy link
Contributor

Ok this LGTM! @annevk I think this one is good to merge? Does @kaf-lamed-beyt need to sign the participation agreement in order for this one to land?

@kaf-lamed-beyt
Copy link
Contributor Author

Awesome! 🚀

I signed the participation agreement already though.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks @kaf-lamed-beyt and congratulations on your first contribution!

And thanks a lot @keithamus (and @bathos!) for helping out, much appreciated!

@annevk annevk merged commit 52a3734 into whatwg:main Jun 12, 2023
2 checks passed
@kaf-lamed-beyt
Copy link
Contributor Author

Ayy!! Thanks, @annevk. I look forward to doing more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Changes that do not affect how the standard is understood.
Development

Successfully merging this pull request may close these issues.

None yet

4 participants