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

[EuiForm] Added invalidCallout prop to allow conditional rendering of error callout #3585

Merged
merged 12 commits into from
Jun 11, 2020

Conversation

shrey
Copy link
Contributor

@shrey shrey commented Jun 10, 2020

Summary

Added an additional boolean showCallout property in EuiForm which the user can set to true if they want the error callout to be shown

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Fixes #478 @snide @cchaos

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

We try to limit our usage of boolean prop types because they're not scalable.

You'll also need to:

  • Add a test for the new prop
  • Update the snapshots
  • Add a changelog entry

src/components/form/form.tsx Outdated Show resolved Hide resolved
src/components/form/form.tsx Outdated Show resolved Hide resolved
src/components/form/form.tsx Outdated Show resolved Hide resolved
@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos Made the changes :)

@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

We try to limit our usage of boolean prop types because they're not scalable.

You'll also need to:

  • Add a test for the new prop
  • Update the snapshots
  • Add a changelog entry

Okay on it

@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos Added testing and updated the changelog could you review this?

@chandlerprall
Copy link
Contributor

jenkins test this

@cchaos cchaos changed the title Made Eui-TopLevel form customisable for callout rendering. [EuiForm] Added invalidCallout prop to allow conditional rendering of error callout Jun 10, 2020
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Can you update the PR description to align with the new prop name? We'll also want to add a quick mention of this prop in the docs example. Can you change the example text to:

Screen Shot 2020-06-10 at 17 38 38 PM

CHANGELOG.md Outdated Show resolved Hide resolved
src-docs/src/views/form_validation/validation.js Outdated Show resolved Hide resolved
src/components/form/form.tsx Outdated Show resolved Hide resolved
src/components/form/form.test.tsx Outdated Show resolved Hide resolved
@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos Made the changes, Could you review it?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3585/

@cchaos
Copy link
Contributor

cchaos commented Jun 10, 2020

@shrey You're still missing a few changes from the review like changing the text of the test and docs example.

@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos Done

@@ -1,5 +1,13 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`EuiForm checks if invalidCallout works 1`] = `
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to update the snapshots again as well.

@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos Updated the snapshot, Could you review it?

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3585/

@shrey
Copy link
Contributor Author

shrey commented Jun 11, 2020

@cchaos Any clue why the tests failed?

@chandlerprall
Copy link
Contributor

15:30:23 Snapshot Summary
15:30:23 › 1 snapshot obsolete from 1 test suite. To remove it, run yarn run test-unit -u.
15:30:23 ↳ src/components/form/form.test.tsx
15:30:23 • EuiForm checks if invalidCallout works 1

Please run yarn test-unit -u to update the snapshot, then add & commit the changes.

@shrey
Copy link
Contributor Author

shrey commented Jun 11, 2020

@chandlerprall Yeah, done.

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3585/

@chandlerprall
Copy link
Contributor

17:45:33 error An unexpected error occurred: "https://registry.yarnpkg.com/nodegit/-/nodegit-0.23.0.tgz: unexpected end of file".

Network error preparing the docker test environment.

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3585/

@shrey
Copy link
Contributor Author

shrey commented Jun 11, 2020

@chandlerprall passed, finally :)

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Sorry @shrey I messed up the props doc syntax and so the props table wasn't showing the prop comment. I have fixed this now and once CI passes (again) I'll merge it down.

Thanks 🎉 !

@cchaos
Copy link
Contributor

cchaos commented Jun 11, 2020

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3585/

@shrey
Copy link
Contributor Author

shrey commented Jun 11, 2020

@cchaos No issues, Tests passed :)

@cchaos cchaos merged commit 3b6bd40 into elastic:master Jun 11, 2020
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3585/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiForm top-level error needs to be customizable
4 participants