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

Scene fix #59

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Scene fix #59

wants to merge 2 commits into from

Conversation

robin22d
Copy link

Changing the configuration of pannellum.viewer() so that the scene is initialised. Therefore all functions that use and rely on scene can be used.

farminf and others added 2 commits March 15, 2019 14:25
develop to master for readme file
This means that function that rely of having scene accessible will now work.
Adding id to pannellum img viewer example.
@codecov-io
Copy link

codecov-io commented Mar 15, 2019

Codecov Report

Merging #59 into develop will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #59      +/-   ##
==========================================
- Coverage     1.21%   1.21%   -0.01%     
==========================================
  Files            6       6              
  Lines         2063    2064       +1     
==========================================
  Hits            25      25              
- Misses        2038    2039       +1
Impacted Files Coverage Δ
src/pannellum/js/pannellum.js 0.14% <0%> (ø) ⬆️
src/elements/Pannellum.js 11.47% <0%> (-0.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a01eba3...72c09ab. Read the comment docs.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 132

  • 0 of 3 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0008%) to 0.851%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pannellum/js/pannellum.js 0 1 0.0%
src/elements/Pannellum.js 0 2 0.0%
Totals Coverage Status
Change from base Build 129: -0.0008%
Covered Lines: 25
Relevant Lines: 2064

💛 - Coveralls

@farminf farminf self-requested a review March 18, 2019 09:00
@farminf
Copy link
Owner

farminf commented Mar 18, 2019

Thanks for the PR... I'll check and merge it later

@robin22d
Copy link
Author

robin22d commented May 22, 2019

Is there anything I else need to do to get this merged in?

Thanks

@farminf
Copy link
Owner

farminf commented May 23, 2019

@robin22d really sorry for the delay
can you explain what is the added value? because I prefer to have "tour" as a new component (like the original pannellum idea) so it is more close to react component approach.
The way that you want would have breaking changes because of sceneId prop I guess, right?
The other thing is modifying the source js of pannellum is not really a good idea IMHO

what do you think?

@robin22d
Copy link
Author

Yes I agree that modifying the source js of pannellum is not really a good idea. But from what I can see there is no way of giving the scene an id therefore a few of the functions cannot be used without changing pannellum. The only updates that would be adding an id and this would not be much of an update.

Do you know of a better way to assign scene ids?

@farminf
Copy link
Owner

farminf commented May 24, 2019

I'm thinking of having a "Tour" component, which will be something like this:

<Tour  firstScene="xxx" ... >
  <Pannellum  id=""... >
    <Pannellum.Hotspot sceneId="" ... />
    ...
  </Pannellum>
  <Pannellum  id=""... >
    <Pannellum.Hotspot sceneId="" ... />
  </Pannellum>
   ...
</Tour>

I sleep on it...

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.

4 participants