-
Notifications
You must be signed in to change notification settings - Fork 187
Conversation
@jjeffryes this is ready to review when you're ready. |
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.
Looks good, just some layout comments.
js/models/profile/Profile.js
Outdated
@@ -80,6 +81,14 @@ export default class extends BaseModel { | |||
addError('vendor', `The vendor value is not a boolean: ${attrs.vendor}`); | |||
} | |||
|
|||
if (typeof attrs.shortDescription !== 'string') { | |||
addError('shortDescription', 'The shortDescription must be provided as a string.'); | |||
} else if (attrs.shortDescriptio > this.max.shortDescriptionLength) { |
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 you left the "n" off of attrs.shortDescription here.
js/start.js
Outdated
}, 300); | ||
}).on('click-manageConnections', () => | ||
app.connectionManagmentModal.open()) | ||
}) |
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.
Should this have the {} and the extra space/lines removed, so it's just new Onboarding()?
<div class="flexCent"> | ||
<div> | ||
<div class="txCtr rowSm"> | ||
<span class="txUp txB clrT2"><%= ob.polyT('onboarding.introScreen.introLine') %></span><i class="btcIcon margRTn"></i> |
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.
There should be a space between the text and the bitcoin icon.
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 gave it some space via CSS since adding in an actual space gives it more space than what the design reflects (haven't pushed yet).
js/languages/en-US.json
Outdated
"introScreen": { | ||
"connectionLbl": "Connection:", | ||
"btnChange": "Change", | ||
"introLine": "A free marketplace. No fees. No restrictions. Earn Bitcoin", |
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.
Earn Bitcoin should have a period at the end like the other phrases, unless we switch the periods to colons or bullets.
I'd prefer bullets, since that's more grammatically correct. Thoughts, @morebrownies ?
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 was just matching the design on that one. If the design change, I imagine @morebrownies could polish this up.
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.
Yeah we should add a period at the end of "Earn Bitcoin". I missed that one.
styles/components/_onboarding.scss
Outdated
|
||
.select2 { | ||
// without this the select2 dropdowns exceed the width of the form on re-render | ||
width: 375px !important; |
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 tested it with this property turned off, and the select2 elements always set themselves to the right width?
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.
Initially it was the right width, but then if I clicked next and back, it would be the wrong with. But... I can't seem to reproduce that anymore. I updated the CSS.
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 need to take out the debug code.
js/start.js
Outdated
@@ -394,7 +334,7 @@ const onboardIfNeededDeferred = $.Deferred(); | |||
|
|||
function onboardIfNeeded() { | |||
isOnboardingNeeded().done((onboardingNeeded) => { | |||
if (onboardingNeeded) { | |||
if (onboardingNeeded || true) { |
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.
Don't forget to remove the debugging code.
closes #510