farminf / pannellum-react

React Component for Pannellum (open source panorama viewer for the web)
MIT License
113 stars 81 forks source link

add support for rendering array of hotspots #66

Closed jstorm31 closed 5 years ago

jstorm31 commented 5 years ago

I need to render an array of dynamic hotspots in a project where I use the library:

<Pannellum ...>
   <Pannellum.Hotspot ... text="static hotspot" />
   {hotspots.map(hotspot => <Pannellum.Hotspot {...hotspot} />}
</Pannellum>

The result has been an error, because current implementation does not take an array as a child of Pannellum component in account.

So I flatmapped children to be a 1D array of hotspots.

codecov-io commented 5 years ago

Codecov Report

Merging #66 into master will not change coverage. The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #66   +/-   ##
======================================
  Coverage    1.21%   1.21%           
======================================
  Files           6       6           
  Lines        2063    2063           
======================================
  Hits           25      25           
  Misses       2038    2038
Impacted Files Coverage Δ
src/elements/Pannellum.js 11.66% <0%> (ø) :arrow_up:

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 cd91004...d83d88b. Read the comment docs.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 138


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/elements/Pannellum.js 0 1 0.0%
<!-- Total: 0 1 0.0% -->
Totals Coverage Status
Change from base Build 137: 0.0%
Covered Lines: 25
Relevant Lines: 2063

💛 - Coveralls
farminf commented 5 years ago

It's already accepting an array, if you need to pass a nested array, you can make it flat before, then pass it to the component, honestly, I don't see any value in this because it is a very edge case.

jstorm31 commented 5 years ago

Yes, I can prepare an array of hotspots and then pass it as a child, but the way above seems more convenient to me. Moreover, the one-line change won't add any complexity and will still work for other cases.

farminf commented 5 years ago

ok, since it hasn't any breaking changes and makes more convenient, I merge this

jstorm31 commented 5 years ago

Great, thanks! Are you going to release it or wait for more changes at once?

farminf commented 5 years ago

It's on develop now, yeah I'll wait since it's minor.