pannellum-react icon indicating copy to clipboard operation
pannellum-react copied to clipboard

Scene fix

Open robin22d opened this issue 6 years ago • 7 comments
trafficstars

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

robin22d avatar Mar 15 '19 19:03 robin22d

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%> (ø) :arrow_up:
src/elements/Pannellum.js 11.47% <0%> (-0.2%) :arrow_down:

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.

codecov-io avatar Mar 15 '19 19:03 codecov-io

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%
<!-- Total: 0 3
Totals Coverage Status
Change from base Build 129: -0.0008%
Covered Lines: 25
Relevant Lines: 2064

💛 - Coveralls

coveralls avatar Mar 15 '19 19:03 coveralls

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

farminf avatar Mar 18 '19 09:03 farminf

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

Thanks

robin22d avatar May 22 '19 12:05 robin22d

@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?

farminf avatar May 23 '19 08:05 farminf

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?

robin22d avatar May 23 '19 20:05 robin22d

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...

farminf avatar May 24 '19 07:05 farminf