Added conditional rendering to description in EuiCard by hetanthakkar · Pull Request #4582 · elastic/eui

hetanthakkar · 2021-02-25T23:23:03Z

This conditional rendering will prevent redundant render caused when description in EuiCard is false

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
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 · 2021-02-25T23:23:05Z

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?

myasonik · 2021-02-26T01:32:39Z

src/components/card/card.tsx

-        </EuiText>
+        {description && (
+          <EuiText
+            id={`${ariaId}Description`}


Wherever in the file we reference this ID, now has to be conditional as well.

(An aria-describedby={id} that doesn't find that id will cause automated accessibility tests to often fail.)

hetanthakkar · 2021-02-26T18:38:37Z

@myasonik I have made changes. but I'm a bit skeptical about my approach. Could you please review my changes

myasonik

@hetanthakkar1 Your approach did technically work at not rendering invalid aria-describedby attributes but it's a little heavy handed in what else it doesn't render 😄

If we move the conditional a little closer to the problem area, we can still render everything else we need to.

myasonik · 2021-02-27T04:12:01Z

src/components/card/card.tsx

+  if (selectable && description) {
    optionalSelectButton = (
      <EuiCardSelect
        aria-describedby={`${ariaId}Title ${ariaId}Description`}


If earlier in the file you define a variable something like:

const ariaDesc = description ? `${ariaId}Description` : '';

Then you can use it in place of everywhere it's used now

Suggested change

aria-describedby={`${ariaId}Title ${ariaId}Description`}

aria-describedby={`${ariaId}Title ${ariaDesc}`}

I thought of doing the same const ariaDesc = description ? ${ariaId}Description : ''; .But I wasn't sure what should value ariaDesc take i.e. whether ariaDesc = ' ' would work!

myasonik · 2021-02-27T04:12:36Z

src/components/card/card.tsx


  let optionalSelectButton;
-  if (selectable) {
+  if (selectable && description) {


We still want to render this content if the description is empty so we can't just not render the whole thing

hetanthakkar · 2021-02-27T05:34:33Z

@myasonik I have incorporated the changes. Could you please review

myasonik · 2021-02-27T05:48:02Z

Jenkins test this

kibanamachine · 2021-02-27T06:10:22Z

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

hetanthakkar · 2021-02-27T06:58:17Z

@myasonik I am just curious to know why eui-ci build fails so frequently! Most probably it's not related to my PR but is there a way we can fix it

hetanthakkar · 2021-03-01T06:25:25Z

@myasonik Is there anything else i need to change in the code!?

cchaos · 2021-03-01T15:27:20Z

I am just curious to know why eui-ci build fails so frequently!

We've been having a lot of timeouts recently due to the increase in build time. If you update from master you should get a most recent PR that should help reduce the timeouts.

myasonik · 2021-03-01T16:11:40Z

@hetanthakkar1 I merged master for you so that we can check CI. Code changes look OK though if no tests fail for this and need updating, we probably should add some.

myasonik · 2021-03-01T16:11:58Z

jenkins test this

kibanamachine · 2021-03-01T16:35:48Z

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

hetanthakkar · 2021-03-01T17:59:07Z

@cchaos

We've been having a lot of timeouts recently due to the increase in build time. If you update from master you should get a most recent PR that should help reduce the timeouts.

Okay got it! Thanks

hetanthakkar · 2021-03-01T19:14:04Z

@hetanthakkar1 I merged master for you so that we can check CI. Code changes look OK though if no tests fail for this and need updating, we probably should add some.

Ohkayy. Thanks @myasonik !

cchaos

Thanks for getting this one started @hetanthakkar1 . There's still some more tasks to complete before we can merge this PR. We usually track these by the checklist that comes pre-filled in the PR summaries, so be sure not to delete that when creating PR's. I've added it back in to your PR summary and crossed off the unapplicable ones.

Since we're now conditionally rendering description we also need to mark the prop as optional. And since this is a pretty major change to the component, we should update at least one of the examples under the Custom children section. Probably just remove the description from the middle (form) one.

Also, as @myasonik suggested, we'll need a test for this as well. You can find the associated testing file ending in test.ts

cchaos · 2021-03-01T20:04:32Z

CHANGELOG.md

 - Removed static `id` from `EuiQuickSelectPopover` ([#4543](https://github.com/elastic/eui/pull/4543))
 - Fixed support sever side rendering for `EuiDataGrid` ([#4540](https://github.com/elastic/eui/pull/4540))
-
+- Fixed redundant render caused in `EuiCard` when `description` is false ([#4546]https://github.com/elastic/eui/pull/4582)


Suggested change

- Fixed redundant render caused in `EuiCard` when `description` is false ([#4546]https://github.com/elastic/eui/pull/4582)

- Made `description` prop of `EuiCard` optional ([#4546]https://github.com/elastic/eui/pull/4582)

This CL entry also now has to move up to the top section under master

chandlerprall · 2021-03-02T16:07:12Z

Since we're now conditionally rendering description we also need to mark the prop as optional.

Do we want to? I kinda like keeping it required with allowing a false or empty string '' value (as it is today), both of which wouldn't render under an if (description) condition. Feels like it's a mechanism to help ensure an implementation had to intentionally avoid a description.

cchaos · 2021-03-02T16:12:55Z

If we go that route, then I think we at least need to be explicit about the boolean case in the prop types and add a prop comment explaining it.

cchaos · 2021-03-02T17:49:24Z

We talked about this in our weekly sync. Since the basic idea here is still wanting to require a description by default, but allow consumers to sort of override with their own children, we'll want to setup an ExclusiveUnion that if children is provided then description is optional.

…scription' as the missing field instead of 'children'

chandlerprall · 2021-03-02T19:21:07Z

@hetanthakkar1 I've pushed a couples commits to make description optional if children is present. @cchaos please check the updated comment for description.

I verified with:

const TestComponent = () => (
  <>
    {/* these are fine */}
    <EuiCard title="A Card" description="A really cool feature." />
    <EuiCard title="A Card" children="A really cool feature." />
    <EuiCard title="A Card">A really cool feature.</EuiCard>
    <EuiCard title="A Card" description="A really cool feature.">
      A really cool feature.
    </EuiCard>

    {/* only this one errors */}
    <EuiCard title="A Card" />
  </>
);

cchaos · 2021-03-02T19:31:12Z

👍 Yup makes sense to me!

I'd still also like to see an update to that middle example under Custom children to remove the description prop.

cchaos · 2021-03-08T17:20:58Z

@hetanthakkar1 I've commented on your other PR. We try to keep questions/conversations in their respective PR's and issues just to make it easy to follow in the future.

hetanthakkar · 2021-03-10T16:22:45Z

@cchaos I have removed that middle example. Could you please review it

cchaos · 2021-03-11T14:44:41Z

Jenkins, test this

kibanamachine · 2021-03-11T15:07:05Z

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

cchaos · 2021-03-11T15:27:12Z

Jenkins, test this

kibanamachine · 2021-03-11T15:51:02Z

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

cchaos

@hetanthakkar1 I've updated this PR to fix more of the checklist items. 👍 From design.

@chandlerprall can you do a pass on the TS?

chandlerprall

Everything looks good, have one small recommendation.

src/components/card/card.tsx

hetanthakkar · 2021-03-11T19:27:25Z

I have made the necessary changes. Could you please review it

cchaos · 2021-03-11T19:38:34Z

Jenkins, test this

kibanamachine · 2021-03-11T20:00:04Z

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

hetanthakkar · 2021-03-12T20:08:07Z

@cchaos could you merge it

cchaos · 2021-03-15T14:07:48Z

could you merge it

Just waiting on @chandlerprall's final review from the change he requested.

cchaos · 2021-03-16T12:53:43Z

I re-checked through the changes including the aria stuff. Looks good to me. I'll rerun Jenkins one more time then merge
Jenkins, test this.

kibanamachine · 2021-03-16T13:18:59Z

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

Added conditional rendering to description in EuiCard

cbf9ffd

Updated CHANGELOG.md for issue elastic#4546

2ffc944

myasonik suggested changes Feb 26, 2021

View reviewed changes

Imposed condition on aria-describedby for the Decription Id

ad626f1

myasonik suggested changes Feb 27, 2021

View reviewed changes

Imposed condition on aria-describedby for the DecriptionId

c5ab93c

Merge remote-tracking branch 'upstream/master' into pr/4582

16d2b54

cchaos suggested changes Mar 1, 2021

View reviewed changes

chandlerprall added 2 commits March 2, 2021 11:54

Make EuiCard[description] optional if children is present

2fbd93d

inverted order of description/children shapes to prefer messaging 'de…

afc45b6

…scription' as the missing field instead of 'children'

This comment has been minimized.

Sign in to view

Removed description from EuiCard's middle example

f99846a

cchaos added 2 commits March 11, 2021 09:50

Merge remote-tracking branch '2_upstream/master' into EuiCard

18e09e5

Fixed spacing, added a test, and fixed changelog

2699180

cchaos added 2 commits March 11, 2021 10:14

Fixed prop descriptions and added more context to example text

d177bf2

Adjusting playground

92db7ec

cchaos requested a review from myasonik March 11, 2021 16:05

cchaos approved these changes Mar 11, 2021

View reviewed changes

cchaos requested a review from chandlerprall March 11, 2021 16:06

chandlerprall suggested changes Mar 11, 2021

View reviewed changes

src/components/card/card.tsx Outdated Show resolved Hide resolved

hetanthakkar force-pushed the EuiCard branch from 9282af9 to 92db7ec Compare March 11, 2021 19:19

Changed EuiText id for description to ariaDesc

e31d6eb

cchaos merged commit fe05502 into elastic:master Mar 16, 2021

cchaos mentioned this pull request Mar 16, 2021

[EuiCard] card's children is an escape hatch but description is still required #4546

Closed

thompsongl mentioned this pull request Apr 7, 2021

Upgrade EUI to v32.0.4 elastic/kibana#96459

Merged

	aria-describedby={`${ariaId}Title ${ariaId}Description`}
	aria-describedby={`${ariaId}Title ${ariaDesc}`}

	- Fixed redundant render caused in `EuiCard` when `description` is false ([#4546]https://github.com/elastic/eui/pull/4582)
	- Made `description` prop of `EuiCard` optional ([#4546]https://github.com/elastic/eui/pull/4582)

Conversation

hetanthakkar commented Feb 25, 2021 • edited by cchaos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

kibanamachine commented Feb 25, 2021

Uh oh!

myasonik Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

hetanthakkar commented Feb 26, 2021

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

myasonik Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

hetanthakkar Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myasonik Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

hetanthakkar commented Feb 27, 2021

Uh oh!

myasonik commented Feb 27, 2021

Uh oh!

kibanamachine commented Feb 27, 2021

Uh oh!

hetanthakkar commented Feb 27, 2021

Uh oh!

hetanthakkar commented Mar 1, 2021

Uh oh!

cchaos commented Mar 1, 2021

Uh oh!

myasonik commented Mar 1, 2021

Uh oh!

myasonik commented Mar 1, 2021

Uh oh!

kibanamachine commented Mar 1, 2021

Uh oh!

hetanthakkar commented Mar 1, 2021

Uh oh!

hetanthakkar commented Mar 1, 2021

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

cchaos Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

chandlerprall commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cchaos commented Mar 2, 2021

Uh oh!

cchaos commented Mar 2, 2021

Uh oh!

chandlerprall commented Mar 2, 2021

Uh oh!

cchaos commented Mar 2, 2021

Uh oh!

This comment has been minimized.

cchaos commented Mar 8, 2021

Uh oh!

hetanthakkar commented Mar 10, 2021

Uh oh!

cchaos commented Mar 11, 2021

Uh oh!

kibanamachine commented Mar 11, 2021

Uh oh!

cchaos commented Mar 11, 2021

Uh oh!

kibanamachine commented Mar 11, 2021

Uh oh!

hetanthakkar commented Feb 25, 2021 •

edited by cchaos

Loading

hetanthakkar Feb 27, 2021 •

edited

Loading

chandlerprall commented Mar 2, 2021 •

edited

Loading