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

Adds collection items #22

Merged
merged 4 commits into from
Oct 5, 2016
Merged

Adds collection items #22

merged 4 commits into from
Oct 5, 2016

Conversation

ro-savage
Copy link
Member

@ro-savage ro-savage commented Oct 4, 2016

  • Adds collection items routes
  • Adds 3 collection items to redux
  • Adds deepzoom to each item (Gigazoom)
  • Adds 3d models to each item (Sketchfab)
  • Adds back button

Closes:

@@ -19,6 +19,8 @@
"dist:copy-static": "cp -r src/static dist/static",
"dist-static:clean": "rm -rf dist_static && mkdir dist_static",
"dist-static:copy-static": "cp -r src/static dist_static/static",
"lint": "eslint ./src/",
Copy link
Member Author

Choose a reason for hiding this comment

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

We can now lint things :)

@@ -0,0 +1,41 @@
import React from 'react'
Copy link
Member Author

Choose a reason for hiding this comment

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

Mixes Route and Redux. Not sure if this is best way to do it. But otherwise I would be creating a route that just has a container than then has the same stuff. Extra layer, didn't seem to be a good reason for it.

@ro-savage
Copy link
Member Author

@nickmask Ready for code review :)

You will need to do an npm install for this to work.

Copy link
Contributor

@nickmask nickmask left a comment

Choose a reason for hiding this comment

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

Nice work, really clean code.

Merge approved with minor changes.

const DeepZoom = (props) => {
return (
<div>
<iframe width="640" height="360" src={props.url} frameBorder="0" allowFullScreen />
Copy link
Contributor

@nickmask nickmask Oct 4, 2016

Choose a reason for hiding this comment

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

  • Need to change width to 100%. Currently cuts off options with the fixed width.
  • Full screen mode currently doesn't work. Looks like an issue with GigaPan, so unsure what we can do here. Full screen mode does work on the 3D models.

const Model3d = (props) => {
return (
<div>
<iframe width="640" height="480" src={props.url} frameBorder="0" allowFullScreen />
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to change width to 100%. Height is also a bit much for the iphone 4, but we can deal with that when we style.

@ro-savage ro-savage merged commit 37410b1 into master Oct 5, 2016
@ro-savage ro-savage deleted the collection-items branch October 5, 2016 03:28
This was referenced Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants