-
Notifications
You must be signed in to change notification settings - Fork 6
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
Replace legacy Mapbox component for Scrollytelling #992
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
skipCompare?: boolean; | ||
} | ||
|
||
export const useAsyncLayers = (referencedLayers: ReferencedLayer[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
[uniqueChapterLayers, asyncLayers] | ||
); | ||
const resolvedLayers = resolvedDatasetsWithStac.map((layer, index) => { | ||
if (layer.status !== DatasetStatus.SUCCESS) return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ : Curious to know if you had a reason to why you got rid of memoization here, if there are no benefits, can probably get rid of it in line 200 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I intended to remove the caching only as I think was obsolete together with the useMemo
, but looking at it again I think it's better to keep the memo part.
Updated.
function Scrollytelling(props) { | ||
const { children } = props; | ||
|
||
const { isHeaderHidden, headerHeight, wrapperHeight } = | ||
useSlidingStickyHeaderProps(); | ||
|
||
const mapContainer = useRef<HTMLDivElement>(null); | ||
const mapRef = useRef<MapboxMap>(null); | ||
const mapRef = useRef<MapRef>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologies that I am catching this error late, but I just noticed that the new scrolly-telling block could not perform setProjection
- I thought it was just a type error as commented (https://github.com/NASA-IMPACT/veda-ui/pull/992/files#diff-af7791d70ec6eacf00bc029b1e7b44b287a9d2c6cc737a56acd4d1b20cda8f1cR306-R307), but mapRef actually is missing set projection
method. I am not sure why, but flagging this issue first.
**Related Ticket:** #1007 ### Description of Changes - Fix projection change bug introduced by #992 ### Notes & Questions About Changes ### Validation / Testing Check projection change on Sandbox Scrollytelling page: https://deploy-preview-1011--veda-ui.netlify.app/sandbox/mdx-scrolly
Related Ticket: 967
Description of Changes
This PR replaces the old Map component with the new Map component for the Scrollytelling block: https://deploy-preview-992--veda-ui.netlify.app/stories/air-quality-and-covid-19
Changes:
Map
and the<Layer />
componentsuseAsyncLayers
in favor of usinguseReconcileWithStacMetadata
TimelineDatasetStatus
becameDatasetStatus
as I feel we could keep this more generic, added aliases)Notes & Questions About Changes
Validation / Testing
{Update with info on what can be manually validated in the Deploy Preview link for example "Validate style updates to selection modal do NOT affect cards"}