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

added possible pattern for dynamic scrolly with Ai2Svelte #59

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

deaxmachina
Copy link
Contributor

Since we talked about it, added the pattern that I've used with the Scroller component when multi-step Ai2Svelte files are involved and imported from Google Docs.
I suggest that we:

  1. Import all Ai2Svelte svelte components and put them in a lookup dictionary, e.g. called aiGraphics where the key is the same as the id in the google doc (so no change to the Google Doc format)
  2. Drop the use of fetchComponent, makeScrollerSteps and instead use a very simple function like so:
  const makeScrollerStepsStatic = (steps, aiGraphics) => {
    const scrollerSteps = steps.map(step => ({
        background: aiGraphics[step.Background],
        foreground: step.Foreground,
      })
    );
    return scrollerSteps;
  };

in order to add the Svelte components in the background for the steps prop of the Scroller.
Note that we'd need to move this function to utils of the Svelte Kit Bluprint.

Then it's more or less the same as before. It also takes care of having multiple Scrollers on the page and not having to make separate look-up dictionaries for each of them.
I updated the example in the docs with this PR. Let me know if this makes any sense as an approach?

@hobbes7878
Copy link
Member

This looks good to me. And I'd be fine with moving the makeScrollerSteps func to utils/ too.

Would be good to check with @pkd2512 and @MinamiFunakoshiTR as well before we pull it in, if we're all happy with teaching this new pattern.

@MinamiFunakoshiTR
Copy link
Contributor

Looks good to me.

One thing I noticed about using the Ai2svelte component in this way is that the the default option, which hides the graphic if ariaHidden is true but there is no ariaDescription, gets annoying. (This default is from this line in the Ai2svelte component)

This got annoying in the 100 days Ukraine project because we did want to use ariaHidden but didn't want to add alt text for each graphic.

So, a few suggestions:

  • Fix the Ai2svelte component so that this condition is gone and the graphic shows up even if aria description is missing. I think the console.warn() should be enough
  • I've helped people manually add alt text to scroller components several times now. Let's revive the conversation in this issue and decide on how we want to add alt text to our scroller component.

@MinamiFunakoshiTR
Copy link
Contributor

And yes! Let's move makeScrollerSteps to utils/. I've had to customise this several times and having it in utils/ would make that much easier.

@deaxmachina
Copy link
Contributor Author

Thanks @hobbes7878 & @MinamiFunakoshiTR
I was wondering then, should I open another PR in the svelte kit bluprint repo itself to add the makeScrollerStepsStatic function to utils, and whether it should be added in addition to the existing makeScrollerSteps function, so that the current functionality can still be used, or as a replacement? I think having both of them at least for now would be better.

@hobbes7878
Copy link
Member

Sorry for delay, @deaxmachina . Yeah, I'd do a PR in the kit, too, and we can coordinate merging both together. And actually, I'd replace makeScrollerSteps. I want to deprecate all our dynamic import patterns, tbh.. Officially declaring them an anti-pattern for regular use :)

@deaxmachina
Copy link
Contributor Author

I opened a PR in the bluprint repo here: reuters-graphics/bluprint_graphics-kit#76

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.

3 participants