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.

Sign up for GitHub

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

Jump to bottom

Bug 1450032 - Convert bottom (secondary) nav bar to ReactJS #3779

Merged
merged 3 commits into from Jul 13, 2018
Merged

Bug 1450032 - Convert bottom (secondary) nav bar to ReactJS #3779

merged 3 commits into from Jul 13, 2018

Conversation

camd
Copy link
Contributor

@camd camd commented Jul 11, 2018 •
edited

Bug 1450032
OK, tests should be fixed now. Thanks for reviewing this!

@camd camd force-pushed the convert-secondary-navbar-to-react branch 4 times, most recently from eb23299 to 6a516bc Compare July 11, 2018 23:59
@camd camd requested review from edmorley and removed request for edmorley July 11, 2018 23:59
@camd camd force-pushed the convert-secondary-navbar-to-react branch from 6a516bc to 923cfdf Compare July 12, 2018 00:16
@camd camd requested a review from edmorley July 12, 2018 00:17
camd
camd commented Jul 12, 2018
View reviewed changes
ui/js/constants.js
'testfailed',
'busted',
'exception',
'success',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reordering these so we don't have to munge them to show the menu in the right order.

@camd camd requested a review from sarah-clements July 12, 2018 00:18
camd
camd commented Jul 12, 2018
View reviewed changes
tests/ui/unit/filters.tests.js
@@ -10,12 +10,6 @@ describe('linkifyBugs filter', function() {
expect(linkifyBugs('Bug 123456'))
.toEqual('Bug <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=123456" data-bugid="123456" title="bugzilla.mozilla.org">123456</a>');
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This functionality isn't used anymore, so I removed it from the filter

@camd
Copy link
Contributor Author

camd commented Jul 12, 2018

Dang. There are a couple legit bugs found in the selenium tests. Working on fixing them now. Please hold off on review, if you've already started. Sorry about that.

@camd camd force-pushed the convert-secondary-navbar-to-react branch 2 times, most recently from d7c800a to 86b0730 Compare July 12, 2018 20:13
@camd
Copy link
Contributor Author

camd commented Jul 12, 2018

I hit some selenium tests that were failing intermittently until I added some wait.until calls in there. So you'll see a few scattered in the tests. I'm hoping this will have the added benefit of making them more stable for all of us. :)

@camd camd force-pushed the convert-secondary-navbar-to-react branch from 1dd359c to 8055239 Compare July 12, 2018 20:41
edmorley
edmorley approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Looks great!

ui/shared/BugLinkify.jsx Outdated

render() {
return (
<ReactLinkify properties={{ target: '_blank' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Anywhere target="_blank" is used, we must also use rel="noopener noreferrer" for security reasons.

ui/partials/main/thWatchedRepo.html
ng-class="{'active': watchedRepo===repoName}"
ng-click="changeRepo(watchedRepo)"
type="button"
title="{{titleText|stripHtml}}">
Copy link
Contributor

@edmorley edmorley Jul 13, 2018 •
edited

Choose a reason for hiding this comment

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

I think stripHtml is now unused

@camd camd force-pushed the convert-secondary-navbar-to-react branch 2 times, most recently from f917d83 to f94abf3 Compare July 13, 2018 16:44
sarah-clements
sarah-clements reviewed Jul 13, 2018
View reviewed changes
ui/job-view/headerbars/SecondaryNavBar.jsx Outdated
}

getSearchStr() {
const ss = this.thJobFilters.getFieldFiltersObj().searchStr;
Copy link
Contributor

Choose a reason for hiding this comment

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

ss is short for searchString presumably?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but probably better to spell it out for clarity.

sarah-clements
sarah-clements reviewed Jul 13, 2018
View reviewed changes
ui/models/treeStatus.js
@@ -30,7 +30,7 @@ export default class TreeStatusModel {
Promise.resolve({
result: {
status: 'error',
message_of_the_day: 'Unable to connect to the <a href="https://mozilla-releng.net/treestatus">TreeStatus</a> API',
message_of_the_day: 'Unable to connect to the https://mozilla-releng.net/treestatus API',
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why this is called 'message_of_the_day'. Sounds like it changes depending on the day :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can, yeah. If there are special conditions about the branch, it will be in this message. I don't think it changes daily, but possibly could. reason would have info about tree closures, if there was one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for answering my questions :)

sarah-clements
sarah-clements reviewed Jul 13, 2018
View reviewed changes
ui/js/services/jobfilters.js Outdated
},
nonfailures: {
value: 'nonfailures',
name: 'non-failures',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does 'non-failures' name have a hyphen and the value does not? (since the other filter groups are consistent with name and value.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, that's an excellent question. The answer is that name and value are vestigial artifacts of how we did things in Angular. We don't need them anymore. So I simplified that structure to just be an object with keys as each group (used by the colored fitler chicklets(dots) on the navbar) with values as arrays of resultStatus.

Thanks for catching that! :)

sarah-clements
sarah-clements approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@sarah-clements sarah-clements left a comment

Choose a reason for hiding this comment

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

Looks good - added a few nit comments/questions.

@camd camd force-pushed the convert-secondary-navbar-to-react branch from f94abf3 to 1381ce2 Compare July 13, 2018 22:27
Cameron Dawson added 2 commits July 13, 2018 15:27
Bug 1450032 - Convert bottom (secondary) nav bar to ReactJS
121f450
Bug 1450032 - Convert term resultState back to resultStatus
4d2429d 
I had wanted to migrate to using "resultState" instead, as it
seemed more descriptive of what it is.  But the filter params
are using "resultStatus" and it would not be worth
the effort to migrate.  It doesn't really matter, but I want to be
consistent to remove confusion, so moving these terms back
to "resultStatus"-ish names.
@camd camd force-pushed the convert-secondary-navbar-to-react branch from 1381ce2 to fef404f Compare July 13, 2018 22:28
@camd camd merged commit bc4e8a7 into master Jul 13, 2018
@camd camd deleted the convert-secondary-navbar-to-react branch July 13, 2018 23:04
@camd
Copy link
Contributor Author

camd commented Jul 13, 2018

Thanks for the excellent feedback, both of you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarah-clements sarah-clements sarah-clements approved these changes

@edmorley edmorley edmorley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@camd @edmorley @sarah-clements