Skip to content

feat/fix(client): refactor certMap and fix date propType#39695

Closed
ShaunSHamilton wants to merge 30 commits intofreeCodeCamp:mainfrom
ShaunSHamilton:feat/refactor-cert-graphql
Closed

feat/fix(client): refactor certMap and fix date propType#39695
ShaunSHamilton wants to merge 30 commits intofreeCodeCamp:mainfrom
ShaunSHamilton:feat/refactor-cert-graphql

Conversation

@ShaunSHamilton
Copy link
Member

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

@gitpod-io
Copy link

gitpod-io bot commented Sep 25, 2020

@ShaunSHamilton ShaunSHamilton force-pushed the feat/refactor-cert-graphql branch from cf0cb82 to 2f607a9 Compare September 27, 2020 23:07
@ShaunSHamilton ShaunSHamilton added the platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. label Sep 27, 2020
@ShaunSHamilton
Copy link
Member Author

Dev Team, this implementation renders the certs on the settings page in the order provided by their order property. However, as it currently is, the legacy certs are not ordered in any particular way.

Is this implementation ok? Or, should it match the current (production) order?

@gitpod-io
Copy link

gitpod-io bot commented Dec 14, 2020

@ojeytonwilliams
Copy link
Contributor

Hey Shaun, apologies for the delay getting to this. Is this PR ready for review?

Dev Team, this implementation renders the certs on the settings page in the order provided by their order property. However, as it currently is, the legacy certs are not ordered in any particular way.

So, this would just be the order Legacy Front End, Legacy Back End and so on appear (after Legacy Full Stack Certification)? I don't think that matters much and we can always change their order properties.

@ojeytonwilliams
Copy link
Contributor

I was thinking about this PR from an internationalisation perspective. What I think we we need to do is add some additional properties to the certificates. The approach here, where properties of the title are used in the logic, is likely to cause problems when the titles are translated. Could we add more properties to the frontmatter and use those instead? For instance using version: 7 and/or isLegacy: true as needed would mean the titles could be freely translated without breaking anything.

@ShaunSHamilton
Copy link
Member Author

@ojeytonwilliams No, this is not ready for review. I keep pushing it back as changes are made to the associated files.

I was thinking about this PR from an internationalisation perspective. What I think we we need to do is add some additional properties to the certificates.

Very good point. I will be able to spend some time looking through this kind of this, these holidays. I would like clarification, though, on where and what I can change/add?

I am not a fan of giving some certs/projects properties, and not others. So, could I adjust all files for consistency, or would this be undesirable?

@ojeytonwilliams
Copy link
Contributor

@ojeytonwilliams No, this is not ready for review. I keep pushing it back as changes are made to the associated files.

I'm working on them too, but I'll fix any conflicts (just ping me in chat if I forget).

I am not a fan of giving some certs/projects properties, and not others. So, could I adjust all files for consistency, or would this be undesirable?

Changing them all is fine by me - there are only 15 certificate files after all. If we were talking about a change that's only relevant for a small number of challenges, we could consider defaulting when that property is absent. Since it's a small number, I agree that consistency is the main thing.

@ojeytonwilliams
Copy link
Contributor

FYI: one thing I'm trying out is adding info like

superBlock: information-security-v7
flag: isInfosecCertV7

to the frontmatter.

@ShaunSHamilton
Copy link
Member Author

@ojeytonwilliams Before I do any more on this, and need to rework with rebasing, I would prefer to get:

  1. feat(client): add project links to certificate #40071
  2. fix(client): give useful error in solutionform #40225

Both of the above merged, as they touch associated files. Will the above be blocked by this Code Freeze?

@raisedadead
Copy link
Member

We are going to be doing a head branch rename. I understand this is a high-priority PR. I can stow this on a separate branch on the main repo for now if you all prefer.

@ShaunSHamilton
Copy link
Member Author

@raisedadead

I understand this is a high-priority PR. I can stow this on a separate branch on the main repo for now if you all prefer.

This is not really high-priority. So, I do not mind whether you close this and move it to a separate branch or just close. Once the related PRs are sorted, I can get back to this with a new branch, if need be.

@raisedadead raisedadead changed the base branch from master to main February 1, 2021 13:11
@raisedadead
Copy link
Member

I have gone ahead and re-targeted this to the main branch that we just created. You may need a rebase before this is actually merged

@ojeytonwilliams
Copy link
Contributor

Hi @ShaunSHamilton nice work on the other two PRs 🎉

I finally got around to converting from .markdown to .yml so there might be a few conflicts. Turned out the conversion was simply a matter of deleting everything that wasn't already yaml, so it should not be a painful rebase. If that proves not to be the case, let me know and I'll try and sort it out.

@ShaunSHamilton
Copy link
Member Author

@ojeytonwilliams Thank you. The conflicts are minimal, and I will probably merge the current changes from main into the curriculum challenge files I changed. Once I have time, I will plough through this again, and find a better way to do what I was originally trying.

@ojeytonwilliams
Copy link
Contributor

Cool, cool - no rush at all.

We can always hop on a call if there's something you'd like to discuss. Otherwise, I'm looking forward to the rework.

@ShaunSHamilton
Copy link
Member Author

Closing as stale, and would hold back TS migration. Idea still useable, in principle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform: learn UI side of the client application that needs familiarity with React, Gatsby etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants