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.

Sign up for GitHub

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

Jump to bottom

(demo-fix)Added width in style.css file #102

Merged
merged 1 commit into from
Jul 14, 2019
Merged

(demo-fix)Added width in style.css file #102

merged 1 commit into from
Jul 14, 2019

Conversation

sidntrivedi012
Copy link
Member

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

  • tests pass -- see README.md for how to run them
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request are descriptively named
  • if possible, multiple commits squashed if they're smaller changes
  • reviewed/confirmed/tested by another contributor or maintainer

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

Please alert developers on [email protected] when your request is ready or if you need assistance.

Thanks!

@welcome
Copy link

welcome bot commented Jul 13, 2019

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

@sidntrivedi012
Copy link
Member Author

I have manually added the width property in the css file in the examples folder. After adding a defined width to the graph div instead of width:100%; the error of Invalid dimensions doesn't show up.

Dhiraj240
Dhiraj240 reviewed Jul 13, 2019
View reviewed changes
examples/capture/capture.css
@@ -1,39 +1,44 @@
.navbar-fixed-top .nav li {
padding-top:3px;
font-family:junction,sans-serif;
padding-top: 3px;
Copy link
Member

Choose a reason for hiding this comment

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

have you manually added these spaces? If not you can use a linter tool Eg: css lint although there are some production level linting tools available.

@Dhiraj240
Copy link
Member

Please add relevant screenshot too.

starkblaze01
starkblaze01 reviewed Jul 13, 2019
View reviewed changes
examples/capture/index.html
@@ -212,7 +212,7 @@

<div class="col-md-10">

<div id="graph" style="width:100%;height:200px;margin:10px 0;"></div>
<div id="graph"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Here the width was set up to 100% because of which the initial width was getting set to 0, so can you please upload the screenshot of the work-flow before and after?. Also, what are the default dimensions div with id #graph is taking up after you removed height and width?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Adding them.

Also, I have added these styles to the style.css file. As of now, I have set the width to 1024 px. I am thinking of using media queries to make the graph responsive. What do you say? Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

#graph {
  width: 1024px;
  height: 200px;
  margin: 10px 0px;
}

Copy link
Member

Choose a reason for hiding this comment

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

Oh you know as long as you're having this discussion here we might as well conclude this within this PR as well, then. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

So the solution was to set width in the stylesheet instead of with inline css?

@sidntrivedi012
Copy link
Member Author

main4

The error that was popping in the demo ^

@sidntrivedi012
Copy link
Member Author

demo
After the fix ^

@sidntrivedi012
Copy link
Member Author

Though one error is still coming related to CORS.

Access to XMLHttpRequest at 'file:///capture/recent_calibrations.json?calibration_id=undefined' from origin 'null' has been blocked by CORS policy: Cross origin requests are only supported for protocol schemes: http, data, chrome, chrome-extension,https.

@sidntrivedi012
Copy link
Member Author

Screenshot 2019-07-14 at 3 41 36 PM

@jywarren
Copy link
Member

jywarren commented Jul 14, 2019 via email

@jywarren
Copy link
Member

jywarren commented Jul 14, 2019 via email

@sidntrivedi012
Copy link
Member Author

sidntrivedi012 commented Jul 14, 2019 •
edited
Loading

@jywarren Yes, it's working while recording video too. I am just posting the screen recording for it.

Also, should I implement media queries in it to make the graph div (the black portion which renders the graph) responsive?

@sidntrivedi012
Copy link
Member Author

demo
@jywarren PTAL.

@jywarren
Copy link
Member

jywarren commented Jul 14, 2019 via email

@jywarren
Copy link
Member

Would you like to open a new PR which adds a button (labelled "Connect to Raspberry Pi") to the start screen, using the following code?

https://github.com/publiclab/infragram/blob/main/pi/index.html#L185-L191

We'll have to adjust this line to correctly send the video data from a raspberry pi camera to the spectral Workbench code:

infragram.options.processor.updateImage(piImage);

@jywarren
Copy link
Member

Actually I'm sorry to ask for that in this PR, it's really a separate issue. I moved my comments and suggestions on that topic back over to #75 (comment)

However this is something I think you could open a new PR for! I'm going to merge this one since you solved the core issue. Great work, and you can open your next one building off this work!

@jywarren jywarren merged commit 5f9f242 into publiclab:main Jul 14, 2019
@welcome
Copy link

welcome bot commented Jul 14, 2019

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to https://spectralworkbench.org in the next few days.
In the meantime, can you tell us your Twitter handle so we can thank you properly?
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

@jywarren
Copy link
Member

🎉🎉🎉👍🏽😁☑️😍💥🦸🏽‍♂️

@jywarren
Copy link
Member

@cryptoclidus does this work for you?

@jywarren jywarren mentioned this pull request Jul 14, 2019
Closed
@starkblaze01 starkblaze01 mentioned this pull request Dec 10, 2019
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dhiraj240 Dhiraj240 Dhiraj240 left review comments

@starkblaze01 starkblaze01 starkblaze01 left review comments

@jywarren jywarren jywarren approved these changes

@harshithpabbati harshithpabbati Awaiting requested review from harshithpabbati

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sidntrivedi012 @Dhiraj240 @jywarren @starkblaze01