Skip to content
This repository has been archived by the owner on Mar 5, 2020. It is now read-only.

Updates to the badges for MozFest #2339

Merged
merged 6 commits into from Oct 17, 2016
Merged

Updates to the badges for MozFest #2339

merged 6 commits into from Oct 17, 2016

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Oct 13, 2016

fixes #2312, fixes #2313

Before this can land:

  • make attachments send data from the RequirementRow to the RequirementsList, to the single-badge component
  • pack multiple bits of evidence correctly before sending it off
  • clean up the UI for evidence because right now offering everything might be necessary but certainly doesn't "look nice".
  • add the final "apply" button back at the bottom of the single-badge page so you can actually send off your application again.
  • remove the placeholders for achieved and pending, because we don't need to show the user's evidence in these views.

@cadecairos cadecairos temporarily deployed to learning-mozilla-org-s-pr-2339 October 13, 2016 21:24 Inactive
@Pomax Pomax temporarily deployed to learning-mozilla-org-s-pr-2339 October 13, 2016 21:27 Inactive
@@ -107,6 +107,8 @@ var BadgePage = React.createClass({
parseBadgeDetails: function(data) {
var bdata = data.badge;

console.log(bdata);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove

@Pomax Pomax temporarily deployed to learning-mozilla-org-s-pr-2339 October 13, 2016 21:28 Inactive
@gideonthomas gideonthomas self-assigned this Oct 13, 2016
@Pomax Pomax temporarily deployed to learning-mozilla-org-s-pr-2339 October 13, 2016 22:02 Inactive
@Pomax Pomax temporarily deployed to learning-mozilla-org-s-pr-2339 October 14, 2016 05:05 Inactive
@Pomax Pomax temporarily deployed to learning-mozilla-org-s-pr-2339 October 17, 2016 18:40 Inactive
@Pomax Pomax changed the title [WIP] Badge rejigger Updates to the badges for MozFest Oct 17, 2016
@Pomax Pomax mentioned this pull request Oct 17, 2016
11 tasks
Copy link
Contributor

@gideonthomas gideonthomas left a comment

Choose a reason for hiding this comment

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

Generally looks good, just a few questions!

render: function () {
var icon = null;

if (this.props.icon !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure I understand this block...does the false value actually mean something? If so, can we comment on that here?

Copy link
Contributor Author

@Pomax Pomax Oct 17, 2016

Choose a reason for hiding this comment

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

yes, it means "no icon, at all" rather than "this icon, or the default icon if we didn't set this prop value"

return (
<div className="apply-send-qualifications">
{/*
<div className="evidence-title">{ this.props.evidence }</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

umm, remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fun fact: uncommenting this in the PR for parsing evidence and criteria as HTML lists, so no need to remove here.

<fieldset>
<input type="file" className="hidden" ref="optionalFile" onChange={this.handleFiles}/>
<label className="control-label">Attach one or more file (if needed):</label>
<button className="btn attach" onClick={this.selectFiles}>click here to pick one or more files...</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Capital 'C'?

return null;
}

return this.state.evidenceFiles.map(e => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we name this evidence?

Copy link
Contributor Author

@Pomax Pomax Oct 17, 2016

Choose a reason for hiding this comment

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

no, because evidence is the full collection of fields. Both the text and the files are evidence as far as Credly cares, hence evidenceText (from the textArea) and evidenceFiles (from the file picker)

Copy link
Contributor

Choose a reason for hiding this comment

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

evidenceFile maybe?

},

handleFiles: function(evt) {
var self = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still needed?

@@ -107,6 +105,10 @@ var BadgePage = React.createClass({
parseBadgeDetails: function(data) {
var bdata = data.badge;

// FIXME: LOGGING ONLY, REMOVE REMOVE REMOVE
console.log(bdata);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

var criteria = [];

// extract evidence as itemized list based on newlines (\n with optional \r) if we can
var splitOnItems = /\n\r?/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is temporary right? Until we settle on a format?

Copy link
Contributor Author

@Pomax Pomax Oct 17, 2016

Choose a reason for hiding this comment

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

yeah this gets removed in the PR for proper parsing of marked up data.

status: status
criteria,
evidence,
date_achieved: bdata.created_at,
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be changed? Isn't it some other property?

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, need to find out which property that is as separate task

@gideonthomas
Copy link
Contributor

lgtm

@Pomax Pomax merged commit 428783e into master Oct 17, 2016
@Pomax Pomax deleted the badge-rejigger branch October 17, 2016 21:46
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.

Badge details not loading Bug on Credentials - giant badge!
3 participants