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

Migrate Storylines to RAMP4 #377

Closed
wants to merge 1 commit into from
Closed

Conversation

mohsin-r
Copy link
Member

@mohsin-r mohsin-r commented Jul 21, 2023

No Pulling.
Ready for full review now.
Worked on jointly with @roryhofland.

This PR is essentially an enormous rebase of our previous RAMP4 demo in January, with a couple of small enhancements thrown in.
Full list of changes:

  • Storylines now uses RAMP4 for map panels
  • gutted any RAMP2 styling that was causing conflicts
  • config files have been converted to RAMP4
  • the Timeslider fixture was ported to RAMP4

There are still a few quirks, some of which may require changes in RAMP4.


This change is Reviewable

@mohsin-r mohsin-r changed the title Migrate storylines to RAMP4 Storylines in RAMP4 Demo Jul 21, 2023
@github-actions
Copy link

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/story-ramp/ramp4/#/en/00000000-0000-0000-0000-000000000000

@mohsin-r mohsin-r changed the base branch from main to ramp4 July 21, 2023 21:09
@mohsin-r mohsin-r changed the base branch from ramp4 to main July 21, 2023 21:09
@mohsin-r mohsin-r force-pushed the ramp4 branch 2 times, most recently from cae57e5 to cc4fc56 Compare July 21, 2023 21:30
@james-rae
Copy link
Member

Map appears really fast. I guess having the story "title / landing" first gives it a few seconds to pull down the library, but from a consumer perspective I would not complain about a lagging load time.

@mohsin-r
Copy link
Member Author

mohsin-r commented Nov 7, 2023

Rebased.

@mohsin-r mohsin-r changed the title Storylines in RAMP4 Demo Migrate Storylines to RAMP4 Nov 7, 2023
Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a bunch of styling issues present on the demo page.

Reviewable status: 0 of 165 files reviewed, all discussions resolved

@spencerwahl
Copy link
Member

It seems like at least some of the issues are causes by a github pages 404 styling block? Hard to tell for sure.

Copy link
Member

@dnlnashed dnlnashed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth mentioning that if you render all possible R4 instances in the demo (including the Map Panel option in the "This Slide Is Dynamic" section), you'll have 9 instances running, which is 18 webgl contexts (2 per instance, 16 being the limit) used and you can see the console warning Too many active WebGL contexts. Oldest context will be lost making all instances 💥
Not an issue to be fixed in this PR, but now we have a Storylines demo we can test the issue with.

A potential fix is doing a null check in RAMP for when the map reacts to an extent change as Mohsin mentioned here, and changing some code in Storylines so that when the instance is visible render it (already done here), but when its not visible on the screen remove it.

Still working on some other solutions that avoid changing Storylines code with ESRI's tryFatalErrorRecovery, but I've tried this with the marketing site and it looks like it works, can try it with storylines once this PR is merged.

Reviewable status: 0 of 165 files reviewed, all discussions resolved

Copy link
Member

@james-rae james-rae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 149 of 162 files at r1, 15 of 15 files at r2, all commit messages.
Reviewable status: 164 of 165 files reviewed, 2 unresolved discussions (waiting on @mohsin-r)


public/scripts/update.js line 11 at r2 (raw file):

const [, , base] = argv;

const fileNames = ['ramp.css', 'ramp.js', 'ramp.esm.js'];

is there a reason for copying the esm version of the library? I don't see it currently in this PR. Unless it's needed, this is just adding another 10mb to the repo every time it runs.


src/components/panels/helpers/time-slider/time-slider.vue line 134 at r2 (raw file):

            props.rInstance.geo.layer
                .allLayers()
                .filter((l: any) => l.supportsFeatures)

maybe might need to also filter out any layer with cosmetic being true? Would need to test but this might do funny stuff to highlighting or any general overlays.

@spencerwahl
Copy link
Member

#392 continued this PR and was merged, closing this one

@spencerwahl spencerwahl closed this Nov 8, 2023
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.

5 participants