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

Adding conditional rendering in EuiCallOut component based on title #3549

Merged
merged 7 commits into from
Jun 4, 2020

Conversation

shrey
Copy link
Contributor

@shrey shrey commented Jun 4, 2020

Summary

I added conditional rendering to the title in the callout function. As extra space was being taken up, when there was a missing title.

Relating to Issue : fixes #3473

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

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

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 4, 2020

💚 CLA has been signed

@cchaos
Copy link
Contributor

cchaos commented Jun 4, 2020

Thanks @shrey !! Can you sign the CLA mentioned below? Just be sure to sign it with the same email address that's linked to your github account.

Jenkins, test this

@cchaos cchaos changed the title Adding conditional rendering in callout component based on title Adding conditional rendering in EuiCallOut component based on title Jun 4, 2020
<div className="euiCallOutHeader">
{headerIcon}
{
title ?
Copy link
Contributor

Choose a reason for hiding this comment

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

As a quick best practices note that we tend to use, is that it tends to be easier to read the output if you set the title render to a variable, letting it stay undefined if title doesn't exist. You can see an example of this earlier in the file with the variable headerIcon that is only given content if iconType is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so should I make another PR fixing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, you can just make the changes locally and push back up to this same branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cchaos okay cool

@shrey
Copy link
Contributor Author

shrey commented Jun 4, 2020

@cchaos I've signed the CLA

@shrey
Copy link
Contributor Author

shrey commented Jun 4, 2020

@cchaos done

@kibanamachine
Copy link

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

@shrey
Copy link
Contributor Author

shrey commented Jun 4, 2020

@kibanamachine Yeah, no specific changes needed in the documentation

shrey added a commit to shrey/eui that referenced this pull request Jun 4, 2020
In lieu of PR elastic#3549 , Updated the change log.
shrey added a commit to shrey/eui that referenced this pull request Jun 4, 2020
In lieu of PR elastic#3549 , Updated the change log.
@shrey
Copy link
Contributor Author

shrey commented Jun 4, 2020

@cchaos I've done the tasks on the checklist, Could you review it once

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.

Great, thanks for going through the checklist!

src/components/call_out/call_out.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@shrey
Copy link
Contributor Author

shrey commented Jun 4, 2020

@cchaos rebased and updated the change log, could you review it once?

@cchaos
Copy link
Contributor

cchaos commented Jun 4, 2020

retest

@kibanamachine
Copy link

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

@shrey
Copy link
Contributor Author

shrey commented Jun 4, 2020

retest

Done, all tests passed

@shrey
Copy link
Contributor Author

shrey commented Jun 4, 2020

@cchaos all tests passed even the snapshots

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.

Yep, sorry when we write retest it's for our Jenkins CI to trigger a new build. But yep, all tests pass! Thank you!

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.

EuiCallOut: title should be required?
3 participants