Skip to content

feat(owners): Display code owners for events#7540

Merged
MaxBittker merged 3 commits into
masterfrom
feat/displaycodeownersevent
Mar 9, 2018
Merged

feat(owners): Display code owners for events#7540
MaxBittker merged 3 commits into
masterfrom
feat/displaycodeownersevent

Conversation

@MaxBittker

@MaxBittker MaxBittker commented Mar 9, 2018

Copy link
Copy Markdown
Contributor

display owners for event
hacky :/ this component should be using our async component pattern and using the tool tip component, but in the interest of getting this ready to dogfood asap I opened this. I'll improve it tomorrow

@MaxBittker MaxBittker requested a review from a team March 9, 2018 06:11
key={`${owner.id}:${owner.type}`}
className="avatar-grid-item tip"
onClick={() => this.assignToActor(owner)}
title={ReactDOMServer.renderToStaticMarkup(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

😬

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is this that bad? better than building a string, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is gross that we send tooltips through bootstrap js

@billyvg billyvg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be nice to have some tests (I'm ok if we do a follow up PR for the other fixes).

render() {
if (!(this.state.owners && this.state.owners.length)) {
let {committers, owners} = this.state;
let showOwners = new Set(this.getOrganization().features).has('internal-catchall');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you're gonna use a mixin, there's getFeatures

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I want to remove the mixins from this component moving forward

</h6>
<div className="avatar-grid">{this.state.owners.map(this.renderCommitter)}</div>
{committers.length ? (
<React.Fragment>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be nice to refactor this into a method or component.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

<React.Fragment>
<h6>
<span>{t('Suggested Owners')}</span>
<small style={{background: '#FFFFFF'}}>Click to assign</small>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

t()

@MaxBittker MaxBittker merged commit 738ab72 into master Mar 9, 2018
@MaxBittker MaxBittker deleted the feat/displaycodeownersevent branch March 9, 2018 19:24
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants