farminf / pannellum-react

React Component for Pannellum (open source panorama viewer for the web)

MIT License

113 stars 81 forks source link

Scene fix #59

Open robin22d opened 5 years ago

robin22d commented 5 years ago

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

codecov-io commented 5 years ago

Codecov Report

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

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

coveralls commented 5 years ago

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	0.0%	-->

Totals
Change from base Build 129:	-0.0008%
Covered Lines:	25
Relevant Lines:	2064

💛 - Coveralls

farminf commented 5 years ago

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

robin22d commented 5 years ago

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

Thanks

farminf commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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