Skip to content

Conversation

DmitiryPotychkin
Copy link
Contributor

  • stop event bubbling after click on component's buttons
  • add ability to pass content in component, display it in line with description text

@DmitiryPotychkin DmitiryPotychkin requested a review from a team as a code owner February 10, 2021 13:58
@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #579 (a1ea721) into main (6c2d647) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #579      +/-   ##
==========================================
- Coverage   85.40%   85.37%   -0.04%     
==========================================
  Files         765      765              
  Lines       15719    15719              
  Branches     1996     1996              
==========================================
- Hits        13425    13420       -5     
- Misses       2261     2266       +5     
  Partials       33       33              
Impacted Files Coverage Δ
...omponents/src/description/description.component.ts 71.42% <33.33%> (-23.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c2d647...a1ea721. Read the comment docs.

data-sensitive-pii
#eventDescriptionText
>
<ng-content></ng-content>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not against using content to pass in the description, but it's an alternative API to passing in description as an input - we shouldn't have both. If we have use cases that need more than a simple string passed in, let's make this the sole API to pass in content and remove the description input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious to know why we need ng-content? Is their an example usage I could see? Is it suppose to wrap any other component apart from a string?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main use cases are links and formatted strings

Copy link
Contributor

Choose a reason for hiding this comment

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

okay.. Links we should handle from outside, but i see the need for formatted strings.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld left a comment

Choose a reason for hiding this comment

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

lgtm - will let @anandtiwary take a pass too

@aaron-steinfeld aaron-steinfeld merged commit 300a80e into hypertrace:main Feb 10, 2021
@github-actions
Copy link

Unit Test Results

    4 files  ±0  234 suites  ±0   14m 13s ⏱️ -37s
837 tests ±0  837 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
841 runs  ±0  841 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 300a80e. ± Comparison against base commit 6c2d647.

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.

3 participants