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

[Playground] EuiBadge, EuiNotificationBadge, EuiBetaBadge #3722

Merged
merged 20 commits into from
Jul 20, 2020

Conversation

anishagg17
Copy link
Contributor

Summary

created playground for badges #3057

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

@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?

@anishagg17 anishagg17 changed the title created playground for badges [Playground] EuiBadge, EuiNotificationBadge, EuiBetaBadge Jul 9, 2020
@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Nice to see an example of an array of playgrounds 👍

src-docs/src/views/badge/playground.js Outdated Show resolved Hide resolved
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.

So excited to have the playgrounds!!! This is my first time looking at the Playground stuff in general so some of these comments are not Badge specific.

  1. Can we try to not add the props that are default to the code block? For instance, the default iconSide for EuiBadge is left. Making this line unnecessary.

Screen Shot 2020-07-14 at 10 00 08 AM

  1. Can you remove the fullScreen option from the code block. They should be quite small, and so full screen is unnecessary.

  2. If a user tries to enter html into a text input, there's an inline error displayed. Can we handle this better?

Screen Shot 2020-07-14 at 10 03 37 AM

  1. The onClickAriaLabel and iconOnClickAriaLabel prop types look wrong to me. They should just be strings.

Screen Shot 2020-07-14 at 10 17 54 AM

  1. Can you add the default children content as the actual content of the display the input?

  2. I noticed that the props table doesn't indicate required props. Can we add this parenthetically like: propName (required)?

  3. I don't think placeholder text is necessary for simply duplicating the prop name. It's easier to scan empty inputs to know what is being applied. I'd remove all placeholders from props like

Screen Shot 2020-07-14 at 10 26 05 AM

  1. The indentation is a bit funky to me it displays as:
<EuiBadge
    iconSide="left"
    iconType="bolt"
    onClick={()=> console.log("Clicked")}
    >
    badge content
</EuiBadge>

But it should be

<EuiBadge
  iconSide="left"
  iconType="bolt"
  onClick={()=> console.log("Clicked")}>
  badge content
</EuiBadge>

src-docs/src/views/badge/playground.js Outdated Show resolved Hide resolved
src-docs/src/views/badge/playground.js Show resolved Hide resolved
src-docs/src/views/badge/playground.js Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Jul 14, 2020

EuiBadge also poses an interesting scenario where:
When passing onClick to EuiBadge, you must also provide onClickAriaLabel

Is there a way we can couple props together? Meaning if:

  1. The user toggles on to "simulate" onClick
  2. By default, the Playground will automatically add a string to onClickAriaLabel
  3. This string is also output into the Modify input for the prop onClickAriaLabel
  4. If the user, deletes the content from the onClickAriaLabel input, the input itself turns invalid because it is required when onClick is added

That is very specific to EuiBadge, but I think a common scenario that will occur when we have props that are required if another prop exists.

@anishagg17
Copy link
Contributor Author

anishagg17 commented Jul 14, 2020

If a user tries to enter html into a text input, there's an inline error displayed. Can we handle this better?

what should be the desired outcome?

The indentation is a bit funky to me it displays as:

This is how react-view renders out the component-code

@cchaos
Copy link
Contributor

cchaos commented Jul 14, 2020

If a user tries to enter html into a text input, there's an inline error displayed. Can we handle this better?

what should be the desired outcome?

Can you sanitize the input and output it as a string?

@thompsongl
Copy link
Contributor

  1. Can we try to not add the props that are default to the code block? For instance, the default iconSide for EuiBadge is left. Making this line unnecessary.
  1. The onClickAriaLabel and iconOnClickAriaLabel prop types look wrong to me. They should just be strings.

Both of these will be resolved automatically when #3554 merges and we have better docgen info to work with

@anishagg17
Copy link
Contributor Author

Can you sanitize the input and output it as a string?

I did that but still it shows error while the html tag is incomplete , should the inline errors be removed ?

@cchaos
Copy link
Contributor

cchaos commented Jul 14, 2020

should the inline errors be removed ?

If we think it's important enough to display the error inline, we should at least use the proper error message display like:

Screen Shot 2020-07-14 at 12 39 28 PM

Otherwise, just output the error in the console.

@anishagg17
Copy link
Contributor Author

When passing onClick to EuiBadge, you must also provide onClickAriaLabel

I have added such validation, but since it is an warning I don't think it is required.

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.

Still found a couple of broken pieces. But overall most of my suggestions look great, thanks for adding them. Also there seems to be a lot of commented out code, you'll want to remove those before merging.

src-docs/src/services/playground/knobs.js Outdated Show resolved Hide resolved
src-docs/src/services/playground/knobs.js Show resolved Hide resolved
src-docs/src/views/badge/playground.js Outdated Show resolved Hide resolved
src-docs/src/views/badge/playground.js Show resolved Hide resolved
@anishagg17 anishagg17 requested a review from cchaos July 16, 2020 19:41
@anishagg17
Copy link
Contributor Author

Also there seems to be a lot of commented out code, you'll want to remove those before merging.

I will surely remove them when it will be ready to merged.

@cchaos
Copy link
Contributor

cchaos commented Jul 16, 2020

retest

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor

Failed CI is a lint error in knobs.js

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Jul 17, 2020

One last thing, I think this error message is too invasive, and honestly not sure it is necessary. It will show on every keypress into the input so as I start to type a correct color like prima for primary every time I press a letter, the error pops pops up. I don't think the popup is necessary at all and I don't know where the UI for it is coming from as it doesn't match any EUI component.

Screen Shot 2020-07-17 at 10 14 05 AM

@thompsongl
Copy link
Contributor

I don't know where the UI for it is coming from as it doesn't match any EUI component

That comes from react-view. In all cases we'll want to either replace it with something in EUI or hide it entirely.

@thompsongl
Copy link
Contributor

jenkins test this

@anishagg17
Copy link
Contributor Author

In all cases we'll want to either replace it with something in EUI or hide it entirely.

that has already been replaced by form validation stuff.

@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

This is ready to go after cleaning up the code comments.

I made #3765 after noticing the console errors in this PR; the cause should be fixed in the component itself.

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit af8575d into elastic:master Jul 20, 2020
@anishagg17 anishagg17 deleted the phase1/badge branch July 20, 2020 17:21
anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
* Break up release.js (elastic#3723)

* Switch release.js to use arguments instead of env vars

* Switch MFA code back to env var so it doesn't leak in CI logs

* Update job definition to use --type arg

* Support breaking up release steps with args

* Break release up to fetch time-sensitive MFA token right before publish

* Strip whitespace from each step

Co-authored-by: Chandler Prall <[email protected]>

Co-authored-by: Chandler Prall <[email protected]>

* Allow prop-setting on the FlexItems within DescribedFormGroup (elastic#3717)

* Allow prop-setting on the FlexItems within DescribedFormGroup

* Add changelog entry

* Honor classes on fieldFlexItem

* Update src/components/form/described_form_group/described_form_group.test.tsx

Co-authored-by: Caroline Horn <[email protected]>

* Update snap name

Co-authored-by: Caroline Horn <[email protected]>

* Re-focus EuiSuperSelect input after making a value change (elastic#3734)

* Re-focus EuiSuperSelect input after making a value change

* changelog

* [EuiStat] Allow customizing the render of the title and description HTML tags (elastic#3693)

* Add titleElement and descriptionElement to EuiStat

* Updated snapshots

* Updated changelog

* Fixed issue with screenreader

* Updated snapshots

* Use screen reader only if title and description is a string

* updated snapshots

* Merge remote-tracking branch 'upstream/master' into fix/stat

* Fixed typo in changelod

* removed titleChildren

* titlechildren as variable

* Update CHANGELOG.md

Remove an extra line left over from a merge resolution

Co-authored-by: Chandler Prall <[email protected]>

* [EuiTable] Expand item action name to allow a function (elastic#3739)

* allow for item -> ReactNode in name

* docs

* CL

* Add ssh keys so new tags can be pushed to Github from Jenkins (elastic#3740)

* Add ssh keys so new tags can be pushed to Github

* Need a vault token before we can pull secrets

* update i18ntokens

* 27.1.0

* Updated documentation.

* Chore/decouple button content (elastic#3730)

* [New] EuiButtonContent

For rendering the contents of buttons, icon (loading spinner) and text wrapper for children

* Use EuiButtonContent in EuiButtonEmpty

* Fixing classNames and disabled states

* Create EuiButtonDisplay for internal usage

* Fix snaps

* ts gymnastics

* Added tests for EuiButtonContent

* More optimization

- Extend EuiButtonContentProps
- Content styles are in button_content.scss

* Restricted list of `element`s

* [Docs] Adding more acccessibility-focused notes and examples (elastic#3714)

* making more a11y callouts in docs

* Apply suggestions from code review

Co-authored-by: gchaps <[email protected]>

* prettier update

Co-authored-by: gchaps <[email protected]>

* [EuiFocusTrap] Use `react-focus-on` (elastic#3631)

* WIP: react-focus-on

* WIP: fix GuideFullScreen

* noIsolation; onClickOutside

* euiflyout snapshot updates

* euiflyout snapshot updates

* euimodal snapshot updates

* euidatagrid snapshot updates

* euicolorpalettepicker snapshot updates

* euisuperselect snapshot updates

* euicollapsible snapshot updates

* euifocustrap snapshot updates

* allow for array snapshots with takeMountedSnapshot

* docs improvements

* default to noIsolation=true and scrollLock=false

* CL

* Fixed bug in EuiComboBox always showing a scrollbar (elastic#3744)

* Fixed EuiComboBox always showing a scrollbar

* Adding the right PR number to CL

* Added useEuiI18n hook (elastic#3749)

* Added useEuiI18n hook

* Updated docs with useEuiI18n hook, added snippets

* Add support to fetch-1i8n-strings or useEuiI18n to match EuiI18n extraction

* Fix up return types for useEuiI18n

* Updated custom eslint i18n rule/package to lint useEuiI18n usages

* changelog

* Remove something I was testing with and lost where I had placeed it.

* [EuiSuperDatePicker] Bypass formatting `null` dates (elastic#3750)

* prevent formatting on null value

* remove unnecessary cast

* account for keyboard nav with null selection

* CL

* Updated EuiComboBox to allow the options list to open for single selection custom options (elastic#3706)

* Fixing includes to return true when object exists in array

* changelog

* Allowing list to open for single selection custom options

* Updated changelog

* PR review

* Improving example

* Improving example

* Addind isClearable

* Improving examples

* Improving explanation text

* Adding note

* Addressing PR issues

* Update src-docs/src/views/combo_box/combo_box_example.js

Co-authored-by: Caroline Horn <[email protected]>

* Update src-docs/src/views/combo_box/combo_box_example.js

Co-authored-by: Caroline Horn <[email protected]>

* Snippet

Co-authored-by: Caroline Horn <[email protected]>

* Add analyze event glyph to EuiIcon (elastic#3729)

* adding analyze event security icon

* updating analyze_event icon

* updating again

* Update CHANGELOG.md

* Update consuming.md (elastic#3769)

* Path has changed and the wiki has not been updated.

* Fix zIndex for two popup ups (elastic#3768)

Clicking both buttons on https://eui.elastic.co/#/tabular-content/data-grid-styling-and-toolbar demo brings up partially hidden popups because their z-index is too low. Increasing by one seems to do the trick.

* [Playground] EuiBadge,   EuiNotificationBadge,   EuiBetaBadge (elastic#3722)

* created playground for badges

* removed commented code

* used validator for iconType and colour

* updated variable name

* removed colour validation

* removed unnecessary imports

* removed default values, fullscreen mode

* suggesstions

* removed placeholder, added required, some badge props set to string

* used actual value of child in text field

* added sanitize function

* fixed unique-key warning

* added validation

* updated to identify the change whenthe state changes

* suggestions

* added onCLick function snippet

* removed error popup by react-view

* removed lint err

* removed commented code

Co-authored-by: Josh Mock <[email protected]>
Co-authored-by: Chandler Prall <[email protected]>
Co-authored-by: Scott Gould <[email protected]>
Co-authored-by: Caroline Horn <[email protected]>
Co-authored-by: Ashik Meerankutty <[email protected]>
Co-authored-by: Greg Thompson <[email protected]>
Co-authored-by: Michail Yasonik <[email protected]>
Co-authored-by: gchaps <[email protected]>
Co-authored-by: Elizabet Oliveira <[email protected]>
Co-authored-by: Marra Sherrier <[email protected]>
Co-authored-by: Alberto Andújar <[email protected]>
Co-authored-by: Yuri Astrakhan <[email protected]>
anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
* created playground for badges

* removed commented code

* used validator for iconType and colour

* updated variable name

* removed colour validation

* removed unnecessary imports

* removed default values, fullscreen mode

* suggesstions

* removed placeholder, added required, some badge props set to string

* used actual value of child in text field

* added sanitize function

* fixed unique-key warning

* added validation

* updated to identify the change whenthe state changes

* suggestions

* added onCLick function snippet

* removed error popup by react-view

* removed lint err

* removed commented code
anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
anishagg17 added a commit to anishagg17/eui that referenced this pull request Jul 20, 2020
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.

4 participants