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

Creating a quote component #38

Merged
merged 7 commits into from
Jul 8, 2015
Merged

Creating a quote component #38

merged 7 commits into from
Jul 8, 2015

Conversation

jodiedoubleday
Copy link
Contributor

What does this PR do? (please provide any background)

  • React Bootstrap doesn't have a Quote component so this PR creates on.
  • Creates a <blockquote> and <q> element
  • Adds all associated Schema
  • Allows you to ass a Cite, Author and Role

What tests does this PR have?

  • Mocha tests check for type, purpose, cite, author and role

How can be this tested?

  • Check the code for coding anti-patterns
  • Run npm run test to run all mocha tests
  • Also run grunt docs and check the example on the UIToolkit docs

Screenshots / Screencast

screen shot 2015-07-07 at 13 00 06

#### What gif best describes how you feel about this work?

url


Developer Definition of Done/Quality Checklist (for PR author to complete BEFORE code review):

Software Engineer or Developer review:

  • I’ve witnessed the work behaving as expected (this could be on the authors machine or screencast).
  • I’ve checked for coding anti-patterns.
  • I’ve checked for appropriate test coverage.
  • I’ve run all the tests locally and they pass.

Software Engineer or project guru review:

  • I’ve witnessed the work behaving as expected (this could be on the authors machine or screencast).
  • I’ve checked for coding anti-patterns.
  • I’ve checked for appropriate test coverage.
  • I’ve run all the tests locally and they pass.

@pmcnr-hx
Copy link
Contributor

pmcnr-hx commented Jul 7, 2015

Taking SE.

@hxtomprice
Copy link

Taking Dev

return (
<q className={classNames(classes)} cite={this.props.cite} itemScope itemType="http://schema.org/CreativeWork" itemProp="text">
{this.props.children}
{(this.props.cite) ? <meta itemProp="citation" content="{this.props.cite}" /> : null}

Choose a reason for hiding this comment

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

should the double quotes be removed here: content="{this.props.cite}"
The output in the html is this as a string: http://grab.by/IDdi

@pmcnr-hx
Copy link
Contributor

pmcnr-hx commented Jul 7, 2015

Minor comment (for future reference, and the you're most probably aware of) regarding the testing instructions: npm test suffices (instead of npm run test) since the npm test command runs the pretest, test and posttest scripts if these exist.

@@ -0,0 +1,8 @@
var example = (
<div>
<p>Gandalf, Captain of the starship enterprise said <UIToolkit.Quote type="inline" author="Gandalf" role="Captain of the Starship Enterprise" cite="The Internet" purpose="success">Use the force Harry</UIToolkit.Quote>
Copy link
Contributor

Choose a reason for hiding this comment

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

Enterprise is a proper noun and should be capitalised (disclaimer: I am not a trekkie). Same probably goes for the Force.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I move the <UIToolkit.Quote> element to the next line in the docs (i.e. replace the space character before the opening tag with a newline), the white space between the previous text and the element disappears in the rendered result. It appears to be the case that any amount of white space other than a single space character results in the white space being removed from the rendered result. Is this a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice spot, i's need to investigate this further, could be the way react works. Ive created a GitHub Issue for this

@hxtomprice
Copy link

Looks good 👍

@pmcnr-hx
Copy link
Contributor

pmcnr-hx commented Jul 8, 2015

All good: 👍.

jodiedoubleday pushed a commit that referenced this pull request Jul 8, 2015
@jodiedoubleday jodiedoubleday merged commit 50ab332 into master Jul 8, 2015
@jodiedoubleday jodiedoubleday deleted the UXUI-280 branch July 8, 2015 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants