-
Notifications
You must be signed in to change notification settings - Fork 605
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
Maintenence badge front end #952
Maintenence badge front end #952
Conversation
This is now ready to be reviewed @carols10cents :). All front end tasks on #704 are contained here. |
#996 looks good to me and bors should be merging it in soon, so please update this when you get a chance! |
@carols10cents I've updated the PR :) |
…crichton Add maintenance badge docs Pending merging of [this PR for crates.io](rust-lang/crates.io#952). This should close off [#704 on crates.io](rust-lang/crates.io#704). I've also updated the badge metadata docs and reordered the fields into groups by build, code coverage, and maintenance. So I've decided to put the list of possible badge options in the documentation, we'll have to find another place to put more detailed descriptions of the available maintenance badges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One bug and a few code simplifications for you to make please :)
Also while you're in there, 2 of the commits have the message "initial commit", which isn't particularly descriptive of what they're doing. I think you could even squash all these commits down into one commit that is "adding maintenance badge front end" or similar since these changes are all one idea.
app/components/badge-maintenance.js
Outdated
tagName: 'span', | ||
classNames: ['badge'], | ||
escapedStatus: computed('badge', function() { | ||
return this.get('badge.attributes.status').replace('-', '--'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I tried out all of the values for the badges, and looking-for-maintainer
didn't load. It looks like it's because this replace
only handles the first hyphen:
Take a look at the replace docs and look for "global" for how to fix this :)
app/components/badge-maintenance.js
Outdated
|| this.get('badge.attributes.status') == undefined) { | ||
return true; | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function could be simplified by returning the value from the comparisons directly, wdyt?
return this.get('badge.attributes.status') === 'none' || !this.get('badge.attributes.status');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not quite sure what !this.get('badge.attributes.status')
does, I mean if it gets the status attribute then it is a not against it? Does the statement return true if the value is undefined
, and false if there is a string returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to change it since it works the same anyway :). Should I add a comment explaining this intruiging JavaScript behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most people experienced with javascript have had experience with how it treats true/false/undefined values, so I don't think this deserves a comment, no.
app/components/badge-maintenance.js
Outdated
}), | ||
status: alias('badge.attributes.status'), | ||
color: computed('badge', function() { | ||
let color = 'undefined'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function could be simplified by removing this color
variable and just returning from each case, something like:
switch (this.get('badge.attributes.status')) {
case 'actively-developed':
return 'brightgreen';
WDYT?
I also don't think we need to explicitly handle anything not listed here; whether we set the color to the string 'undefined' or whether we don't set a color, the badge is going to be broken anyway. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I agree, I've gone ahead and removed handling it.
app/components/badge-maintenance.js
Outdated
return color; | ||
}), | ||
text: computed('badge', function() { | ||
return 'Maintenance intention for this crate'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this text
function isn't really computing anything, right? It's not using badge
for anything... I think this could just be text: 'Maintenance intention for this crate',
like tagName
and classNames
are defined.
Argh this has been a nightmare :(. I'm not sure why there are other changes in this PR, I rebased with the latest changes from master and now I have this. Ok so how this mess happened was when I squashed my commits, I actually squashed all of them back to a bors merge commit rather than my first commit working on this, meaning I lost the changes when I rebased everything again. Now I've learned how to do git squashes without breaking things :). |
Hm, i'm not sure why github is still showing 9 commits... i'm going to try rebasing again |
weird, github, wat r u doin |
app/components/badge-maintenance.js
Outdated
classNames: ['badge'], | ||
escapedStatus: computed('badge', function() { | ||
let regex = /-/g; | ||
return this.get('badge.attributes.status').replace(regex, '--'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, in my opinion, this would actually be clearer if the regex is inline rather than in a variable, so .replace(/-/g, '--')
. That way, what we're replacing and what we're replacing it with is closer together and easier to read. Can you elaborate on why you have it in a variable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it didn't occur to me I could do it this way too :). The inline approach does look neater, I've changed it to be this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one last question :)
Alright the commit history cleaned itself up :) |
Once this is merged, I believe #704 will be finished since the cargo manifest docs have been merged :) |
Looks great now!! Thanks!! bors: r+ |
952: Maintenence badge front end r=carols10cents Start front end work for #704 by creating a maintenance badge component. I'm not quite sure what the alias() function does in ember.js, think it takes out the attributes from the badges for a crate? I'm going to work on all the frontend changes here, since the tests failed when I attempted to add the fixture without the badge component. NOTE: If #996 is approved, I'll have to change this PR to work with it.
Build succeeded |
Thanks for your help on this @carols10cents :) |
Start front end work for #704 by creating a maintenance badge component. I'm not quite sure what the alias() function does in ember.js, think it takes out the attributes from the badges for a crate?
I'm going to work on all the frontend changes here, since the tests failed when I attempted to add the fixture without the badge component.
NOTE: If #996 is approved, I'll have to change this PR to work with it.