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

feature/JENKINS-46913-result-item-links * Linkify URLs in ResultItem headers #1400

Merged
merged 2 commits into from Sep 19, 2017

Conversation

sophistifunk
Copy link
Collaborator

Linkify URLs in ResultItem headers

Description

See JENKINS-46913.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@sophistifunk
Copy link
Collaborator Author

screen shot 2017-09-18 at 6 41 33 pm

@@ -41,6 +41,7 @@
"url": "https://github.com/jenkinsci/jenkins-design-language.git"
},
"dependencies": {
"linkifyjs": "2.1.4",
Copy link
Member

Choose a reason for hiding this comment

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

@sophistifunk this one? https://github.com/SoapBox/linkifyjs/blob/master/package.json so doesn't bring anything along, which is nice, right? (ie fairly small)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah there's not a lot of code in there. It has a dependency on JQ, but only for functions we're not using so we can just ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I eyeballed the deps too and looks fine. I think as long as we verify that the bundles aren't bloating up after this change we are good. They do strange things sometimes :/

@sophistifunk
Copy link
Collaborator Author

I can't seem to see what's actually going wrong here :-/

Copy link
Contributor

@cliffmeyers cliffmeyers left a comment

Choose a reason for hiding this comment

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

LGTM 🐝 Just minor comments.

@@ -78,6 +79,11 @@ export class ResultItem extends Component {
}
};

urlClicked = (e: Event) => {
console.log('urlClicked');
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just make sure we remove that before PR merge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch, cheers

@@ -41,6 +41,7 @@
"url": "https://github.com/jenkinsci/jenkins-design-language.git"
},
"dependencies": {
"linkifyjs": "2.1.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

I eyeballed the deps too and looks fine. I think as long as we verify that the bundles aren't bloating up after this change we are good. They do strange things sometimes :/

@michaelneale
Copy link
Member

Nice - this works great. Wire it in and should go for it. Very nice.

Copy link
Collaborator

@NicuPascu NicuPascu left a comment

Choose a reason for hiding this comment

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

LGTM, I'm usually not a big fan of stopping event propagation but in this case it probably is the best way to go

@sophistifunk sophistifunk merged commit 0e57619 into master Sep 19, 2017
@sophistifunk sophistifunk deleted the feature/JENKINS-46913-result-item-links branch September 19, 2017 07:19
@michaelneale
Copy link
Member

@sophistifunk doesn't this need to be published/incorporated to take effect?

@sophistifunk
Copy link
Collaborator Author

@NicuPascu it's just to stop the expando from opening / closing when you click a link. It's the easiest way to do it.

@cliffmeyers
Copy link
Contributor

cliffmeyers commented Sep 19, 2017

Actually I think in the future we should be sure to publish every package change as part of the PR so it can go through ATH. Otherwise we are just pushing that risk onto the next developer who actually needs to publish. Am quite confident this change won't be an issue, but.. famous last words :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants