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.

Sign up for GitHub

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

Jump to bottom

Add explanation of asterisk for required fields to example forms and documentation #820

Merged
merged 13 commits into from
Nov 22, 2024
Merged

Add explanation of asterisk for required fields to example forms and documentation #820

merged 13 commits into from
Nov 22, 2024
+10 −2

Conversation

joshfarrant
Copy link
Contributor

@joshfarrant joshfarrant commented Nov 15, 2024 •
edited
Loading

Summary

  • Add explanation of asterisk (*) to both example forms and documentation.
  • Simplified FormControl validation docs example (commit).
  • Fixed bug where TextInput width would expand to length of validation message (commit).

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • UI Changes contain new visual snapshots (generated by adding update snapshots label to the PR)
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 15, 2024 •
edited
Loading

🟢 No design token changes found

@joshfarrant joshfarrant marked this pull request as ready for review November 15, 2024 14:39
Copilot
Copilot AI reviewed Nov 15, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • apps/docs/content/components/FormControl.mdx: Language not supported

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
@joshfarrant joshfarrant changed the title Joshfarrant/formcontrol required label Add explanation of asterisk for required fields to both example forms and documentation. Nov 15, 2024
@joshfarrant joshfarrant changed the title Add explanation of asterisk for required fields to both example forms and documentation. Add explanation of asterisk for required fields to example forms and documentation Nov 15, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 15, 2024 •
edited
Loading

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 15, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: c2b78c1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

rezrah
rezrah reviewed Nov 15, 2024
View reviewed changes
apps/docs/content/components/FormControl.mdx
@@ -328,6 +328,8 @@ Pass the `fullWidth` prop to `FormControl` to provide additional behavior and st

Pass the `required` prop to `FormControl` to provide additional behavior and state context to all `children`, rather than the input only.

When marking a field as required, it is recommended to also provide a corresponding message at the start of the form informing the user that "_all fields marked with an asterisk (\*) are required_".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Could we also update the form example further up the page please?

Screenshot 2024-11-15 at 16 26 11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good spot, thanks.

I'd searched for all <form> tags but looks like that was missing one. I've wrapped it in one now and made the required change 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @joshfarrant.

@simmonsjenna here's a sample of the required field statement we need Site to add to all form designs going forward. Let us know if you have any further guidance on spacing, positioning or colors. We can fix in a follow up.

Screenshot 2024-11-21 at 10 45 15

Link

Copy link

Choose a reason for hiding this comment

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

@rezrah LGTM

@joshfarrant joshfarrant force-pushed the joshfarrant/formcontrol-required-label branch from e5cd4c2 to 127844e Compare November 19, 2024 11:59
Copilot
Copilot AI reviewed Nov 20, 2024
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 10 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • apps/docs/content/components/FormControl.mdx: Language not supported
  • packages/react/src/forms/TextInput/TextInput.module.css: Language not supported

Tip: Turn on automatic Copilot reviews for this repository to get quick feedback on every pull request. Learn more
rezrah
rezrah reviewed Nov 21, 2024
View reviewed changes
...monial.visual.spec.ts-snapshots/Visual-Comparison-Testimonial-Testimonial-Avatar-1-linux.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea what's going on with these? Presumably it's the avatar being switched to a local fixture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I'm not sure, that's all I can imagine but I don't understand why that would trigger an update 🤔

@joshfarrant
Revert "fix bug where TextInput width would expand to length of valid…
5a6f749 
…ation message"

This reverts commit 3bbd80c.

This fix will be added in its own PR.
@joshfarrant
update snapshots
d2a3582
rezrah
rezrah approved these changes Nov 22, 2024
View reviewed changes
Copy link
Collaborator

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @joshfarrant

@joshfarrant joshfarrant merged commit 0a41132 into main Nov 22, 2024
18 checks passed
@joshfarrant joshfarrant deleted the joshfarrant/formcontrol-required-label branch November 22, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@simmonsjenna simmonsjenna simmonsjenna left review comments

@rezrah rezrah rezrah approved these changes

@danielguillan danielguillan danielguillan approved these changes

Assignees
No one assigned
Labels
update snapshots
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshfarrant @danielguillan @simmonsjenna @rezrah