CodeYourFuture / curriculum

The CYF Curriculum
https://curriculum.codeyourfuture.io
Other
34 stars 45 forks source link

Introduce taxonomy equivalent of menu map #1187

Open illicitonion opened 1 week ago

illicitonion commented 1 week ago

What does this change?

Introduce taxonomy equivalent of menu map

This produces the same view (sharing code to do so), but allows using a taxonomy instead of a list of menus. The benefit here is a taxonomy gives you a place (the taxonomy term page) to define metadata that may be useful in presenting each term (e.g. attaching a description to each term).

The equivalences between menu and taxonomy maps are:

Concept Menu concept Taxonomy concept
A category (e.g. a column to display in the default map view) A menu A term
A value in the category (e.g. a a card to show in the column) A page in a menu A value within a term

There are some interesting compatibility questions in here which I will comment on in the PR I'm raising for this.

Note: This currently doesn't actually define any interesting metadata (other than weight) for each term in the tracks taxonomy - I will send this through as a follow-up PR, to separate out "the idea of a taxonomy map" from "the details of this particular taxonomy".

Common Theme?

See description above, and line comments for specific considerations.

Org Content?

Updates org-cyf-tracks to use a taxonomy not a menu-map.

Checklist

Who needs to know about this?

cc @CodeYourFuture/dpg-team - I've left comments with some compatibility questions that seem relevant to the releases discussions.

netlify[bot] commented 1 week ago

Deploy Preview for cyf-common canceled.

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-common/deploys/672f5f8893f1d80008d570d6
netlify[bot] commented 1 week ago

Deploy Preview for cyf-curriculum ready!

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-curriculum/deploys/672f5f88b31477000811b0aa
Deploy Preview https://deploy-preview-1187--cyf-curriculum.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
1 paths audited
Performance: 85 (πŸ”΄ down 12 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 week ago

Deploy Preview for cyf-programming ready!

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-programming/deploys/672f5f8822173100088ea44d
Deploy Preview https://deploy-preview-1187--cyf-programming.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
1 paths audited
Performance: 98 (🟒 up 13 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 90 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 week ago

Deploy Preview for cyf-sdc ready!

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-sdc/deploys/672f5f884714290008a61199
Deploy Preview https://deploy-preview-1187--cyf-sdc.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 week ago

Deploy Preview for cyf-launch ready!

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-launch/deploys/672f5f888f50e300081a7d76
Deploy Preview https://deploy-preview-1187--cyf-launch.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 week ago

Deploy Preview for cyf-tracks ready!

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-tracks/deploys/672f5f88db48af000803117b
Deploy Preview https://deploy-preview-1187--cyf-tracks.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 week ago

Deploy Preview for cyf-piscine ready!

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-piscine/deploys/672f5f88e730cd00081c75bf
Deploy Preview https://deploy-preview-1187--cyf-piscine.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
1 paths audited
Performance: 85 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: 60 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 1 week ago

Deploy Preview for cyf-itd ready!

Name Link
Latest commit 0a92a9828291aed581b9d08152112d740fd173e0
Latest deploy log https://app.netlify.com/sites/cyf-itd/deploys/672f5f88d5c12c00081274db
Deploy Preview https://deploy-preview-1187--cyf-itd.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: 60 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

daslerr commented 1 week ago

@illicitonion I don't see any comments in the PR other than the inline comments in the code, and I didn't see any specific questions in those. In general, this all seems reasonable to me. Seems like a reasonable change, and including some backwards compatibility with a warning also seems reasonable to me. Is there something more specific I should be giving feedback on? I should say that for the draft of the automated release note, we adapted a GitHub template for repos using semantic versioning that essentially equates "breaking changes" with major version and any other changes were minor versions. That also seems reasonable to me, but I'm happy to adapt to whatever suits CYF's philosophy. That's possibly a discussion that's tangential to this PR, though.

illicitonion commented 1 week ago

@illicitonion I don't see any comments in the PR other than the inline comments in the code, and I didn't see any specific questions in those. In general, this all seems reasonable to me. Seems like a reasonable change, and including some backwards compatibility with a warning also seems reasonable to me. Is there something more specific I should be giving feedback on?

A reasonable question!

I think my specific questions are:

  1. At some point we want to remove support for the legacy map frontmatter. At what point do you think that should be? Which I guess ends up being the question: How many minor releases should we wait, or how much time should we wait, between making a backwards-compatible shim and turning it into a breaking change?
  2. What do we want to consider as a breaking change? It feels like stopping calling any existing partial, renaming any partial, and changing the expected params of any partial are all definitely breaking changes?
  3. When we remove support for the map frontmatter, do we want to add an explicit {{ if .Params.map }} {{ errorf "map is unsupported" } branch, or do we want to just ignore any handling for it and fall back to whatever the layout would be even if map had never been a special thing?
  4. More broadly, do we want to explicitly require opting in to specific layout types to avoid this problem in the future (i.e. instead of if map { render map } else { render timeline} do we want if map { render map } else if timeline { render timeline } else { errorf "all pages must choose what kind of layout they are" }?) This would require every page to opt in to how it wants to be rendered (rather than some pages doing so and others falling back to a default). I think @SallyMcGrath has currently been modelling the map view as "just some frontmatter you can enable" (which is why it's documented in https://common.codeyourfuture.io/common-theme/front-matter/#map), but I think I model it as "you're changing the entire layout of a page, you just happen to be doing so via some frontmatter"... If we do the else { errorf way, removing an unsupported type automatically becomes a breaking change at build time, rather than defaulting to falling back to something else (and us needing to handle it specially if we want to make it actually break).
daslerr commented 1 week ago

Those are indeed more specific. :)

So first, let me say that from a community perspective, I'm of the opinion that it's much more important to decide on a release cadence and communicate it well than whatever the actual cadence is. I haven't been around CYF long enough to know what you guys would consider a reasonable schedule given your staff and resource constraints, so I don't think that's a determination I can make. Also, with my former PM hat on, I feel like this is straying into product decision territory. I think it would be best to loop in @kfklein15 on this discussion.

My particular (by no means definitive) opinions on your questions:

  1. My vague answer to "how long should we wait" is "a reasonable time for your core users to make the switch". In other contexts, I would maybe look at usage data to figure this out, but I'm not sure that's relevant here. For the users you expect to have, is it maybe more relevant to say "we'll continue to support the old way for a year [or some amount of time rather than number of minor releases]"?
  2. Sure, those all seem like breaking changes to me, but I may not be the best person to determine that.
  3. I don't think you'd need to leave explicit errors for legacy things that are unsupported. I think the documentation and the release notes should indicate that you're sunsetting a particular feature/function/whatever and when that will happen, and I think it's reasonable to include the warnings you've already included for the transition period, but when the specified time to sunset has come, it's just over and done. At this point, my understanding is that only one other organization is using common, so I think best case it's more likely that you'd have new people popping in to use the theme as it currently stands than you would have people looking for features they used to be able to use.
  4. This is a bit in the weeds from a development perspective for my skillset, I'll admit. But my gut feeling is that it doesn't seem necessary to opt-in to layout types specifically. But I'm happy to let other people have that fight.

I hope it's a bit helpful to get a relative outsider's opinion, but again, I don't think I'm the right person to be dictating these decisions. I'm happy to document and support whatever direction CYF would like to go with a release cadence.

I don't think the answers to these questions should hold up merging a PR, personally, but I'll wait for you to confirm that this is a satisfactory "review" before I approve it. I don't want to automerge anything you guys aren't ready for.

illicitonion commented 1 week ago

I think that all makes sense, thanks @daslerr!

I'll leave for @SallyMcGrath to review and we can merge this - thanks for talking through this as a case study with me!