add resources page

[aram-cinnamon]

https://github.com/FRUK-Simulator/Simulator/issues/272

Add secondary/tertiary buttons, progress div to lesson/resources card Add lesson/resources card to resources page

Styled according to https://lucky-smakager-43f6a1.netlify.app/#components-resource-cards https://lucky-smakager-43f6a1.netlify.app/#forms-buttons-secondary https://lucky-smakager-43f6a1.netlify.app/#forms-buttons-tertiary

[owen26]

as part of reviewing this PR, I would also propose that we introduce a non-UI component say called AssetManager that will be the central place to load those assets

I've done something as part of my challenges page implementation, not sure if you are referring to something similar.

https://github.com/FRUK-Simulator/Simulator/blob/main/src/core/data-loader.ts

[yono10]

as part of reviewing this PR, I would also propose that we introduce a non-UI component say called AssetManager that will be the central place to load those assets

I've done something as part of my challenges page implementation, not sure if you are referring to something similar.

https://github.com/FRUK-Simulator/Simulator/blob/main/src/core/data-loader.ts

I think not exactly (unless I missed it somewhere), as in there I see ChallengeCard still accepting a plain url as a parameter - which means it doesn't own that logic (of what makes up an icon image for a Challenge), and that any usage site can just pass in any image into it.

What I was thinking/proposing, would be somethign like (total pseudocode!):

ChallengeCard {
  let iconSvg = when (challengeType) {
    is A -> AssetManager.Shapes.Icons.ChallengeA,
    is B -> AssetManager.Shapes.Icons.ChallengeB,
    is C -> AssetManager.Shapes.Icons.ChallengeC,
  } 
  ...
}

AssetManager {
  Shapes {
    Icons {
      ChallengeA = ... <svg lookup/import>,
      ChallengeB = ... <svg lookup/import>,
      ...
    }

    Backgrounds {
      ...
    }
  },
  ...
}

[owen26]

I think not exactly...

Right.. Cuz I see those cover images as part of lessons/challenges/resources data structure no difference to other fields for example titles and descriptions. If we later add more lessons to the dataset then it's likely there will be new cover images attached to them each representing some nature of the lesson itself. This is unlike generic reusable images like background patterns for example.

So I would argue asset lookup based on type is not necessary because each image is unique and represents only the specific lesson, so it makes more sense to be part of the lesson object.

But ofc I'm open to further discussion :)

[yono10]

I think not exactly...

Right.. Cuz I see those cover images as part of lessons/challenges/resources data structure no difference to other fields for example titles and descriptions. If we later add more lessons to the dataset then it's likely there will be new cover images attached to them each representing some nature of the lesson itself. This is unlike generic reusable images like background patterns for example.

So I would argue asset lookup based on type is not necessary because each image is unique and represents only the specific lesson, so it makes more sense to be part of the lesson object.

But ofc I'm open to further discussion :)

OK yes I had missed that those images will in fact need to be data driven, so will need to come from database indeed. Basically my concern is that the components shouldn't become just generic containers when they have been decidedly called "Lesson Card", "Challenge Card" etc, so they should be by design quite self-sufficient in terms of implementing those characteristics. But yes where the pieces of information are actually those that are data driven, then sure they need to be passed in. (still alternatively though, we could also let them consult something like AssetManager.getLessonBackground() passing in lesson id, if there the urls are standardized into a certain known location, as opposed to being made as part of the lesson data).

Regardless, for non data driven ones though, I do think they should be part of the component implementation details, and not let the callers worry about passing them in.

[yono10]

I think not exactly...

Right.. Cuz I see those cover images as part of lessons/challenges/resources data structure no difference to other fields for example titles and descriptions. If we later add more lessons to the dataset then it's likely there will be new cover images attached to them each representing some nature of the lesson itself. This is unlike generic reusable images like background patterns for example. So I would argue asset lookup based on type is not necessary because each image is unique and represents only the specific lesson, so it makes more sense to be part of the lesson object. But ofc I'm open to further discussion :)

OK yes I had missed that those images will in fact need to be data driven, so will need to come from database indeed. Basically my concern is that the components shouldn't become just generic containers when they have been decidedly called "Lesson Card", "Challenge Card" etc, so they should be by design quite self-sufficient in terms of implementing those characteristics. But yes where the pieces of information are actually those that are data driven, then sure they need to be passed in. (still alternatively though, we could also let them consult something like AssetManager.getLessonBackground() passing in lesson id, if there the urls are standardized into a certain known location, as opposed to being made as part of the lesson data).

Regardless, for non data driven ones though, I do think they should be part of the component implementation details, and not let the callers worry about passing them in.

Obviously much time has elapsed since, and some side discussions has happened too. So just to summarise here, that my suggestion of AssetManager is no longer very essential at this point (even in terms of for the statically loaded assets, ie. non data driven - I think doing that would still be ideal, but perhaps we can improve that later too). For the data driven ones, as previously advised by @owen26 , let's make the component accept the url in, so in the caller site once they have loaded it from the data source, they can pass in the url accordingly.

On the other hand - @aram-cinnamon In the interest of moving this PR forward though, could you look at addressing some of the other outstanding topics that have been raised? @owen26 might want to add, but from what I can tell here are the outstanding items:

• Could you please extract SVG assets out of the components, and using imports instead.
• Let's not use variables inside the SVGs, especially for toggling on/off different parts of the SVG when they can/should just be split into multiple assets imported in separately.
• The buttons on the LessonCard will be required to perform different navigations. Originally I had this as "call2actionText/call2actionDest" pair (with the thinking that we'll just add more such pairs for the secondary and tertiary ones), but that was removed by one of @owen26's changes in favour of the "lessonId". I still think we need that more generic flexibility, because now you have more buttons, they will need to perform different navigations, so I think we need to accept those in as arguments, like my original design. (minor note: call2action stands for "Call to action" 🙂 )

[owen26]

feel free to re-request me review when there are updates :)

[owen26]

Had another pass again, thanks @aram-cinnamon for clarifying some of the points I raised.

So I guess now the only outstanding thing for me is about the "secondary-download" button type. Per my comment, I'd vote for making button accepts icon to be passed in, instead of baking in this specific "download type". @owen26 Maybe you can do the tie breaker 🙂

(Also - I have marked as resolved some of the comments I was involved in and have since moved on, I have left those raised by @owen26 and remain unanswered, so you might want to check those directly with him)

Yes I would strongly advice not to implement secondary-download, but instead the icon should be part the label

<Button type="secondary">
  <DownloadIcon />
  Download
</Button

[aram-cinnamon]

do you also have to approve @immutable-pro ? @cmanolescu ?