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

fix(nav): Curriculum link should use relevant language #101

Closed
wants to merge 9 commits into from

Conversation

erikasby
Copy link
Contributor

@erikasby erikasby commented Jul 6, 2023

Checklist:

Closes freeCodeCamp/freeCodeCamp#40372

api.onPageChange() works funny in local environment, mostly it refreshes, but sometimes it does not work for some reason, there happen some network errors and then bugs start to appear, sometimes it gets triggered then 2-3 times

@erikasby erikasby requested a review from a team as a code owner July 6, 2023 21:17
Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

Please avoid editing off of GitHub directly. It is a terrible idea and can lead to issues like formatting errors as seen in the checks here.

@raisedadead raisedadead changed the title Fix forum's curriculum nav item link to include relevant language of the curriculum fix(nav): Curriculum link should use relevant language Jul 6, 2023
@erikasby erikasby requested a review from raisedadead July 7, 2023 06:14
@raisedadead raisedadead requested a review from ahmaxed July 7, 2023 06:33
@ahmaxed
Copy link
Member

ahmaxed commented Jul 26, 2023

@erikasby, thanks for creating the pr and doing the research. I uploaded the theme to our discourse instance, and here are my observations:

  1. On the forum page, I get the error: properties of undefined (reading 'name')
  2. The Visit the curriculum does not show up by default on the desktop view.
  3. The Visit the curriculum shows up when the window is resized to the mobile view.
  4. When the button shows up, it links to /learn at all times. (even after changing category and reloading)
  5. Would it be possible to have a concise language object instead of having two.
english: learn
spanish: espanol/learn

6: '/learn' could be extracted to the logic instead of having it repeated in the settings.
7: english language is the default so it can be removed from the lists.
8: Since curriculum_src is not pointing to the curriculum, a more descriptive name could be used.
9: Since english is the default, the href variable could be directly set to the english curriculum url. href should only be reset when category matches one of the curriculum languages.
10: If api.onPageChange() is not stable, we would need to use another function.
11: Could you try using the theme creator to test this fix on a discourse instance. It is much better than a local version as it populated with user generated content.

@erikasby
Copy link
Contributor Author

@ahmaxed I found an easier way to update href by just querying the curriculum-nav and updating href on api.onPageChange().

I also updated settings.yml with your suggestions, but I can't remove the language from settings.yml, because, as it is mentioned in that file now, languages need to be in both curriculum_slug and languages, because category name in forum and curriculum learn slug not always correspond to each other; for example portugues and portuguese.

@ahmaxed
Copy link
Member

ahmaxed commented Aug 1, 2023

Thanks for making the changes.

I uploaded the changes to the preview theme, and still get the Cannot read properties of undefined (reading 'name').
Screenshot 2023-08-01 at 12 20 37 PM

Additionally, the slug does not change when post's category change. This might be because the displayed categories are capitalized or that they are more than one. Not sure.

Let me know if the slug changes smoothly on your side, so I could dig deeper.

@erikasby
Copy link
Contributor Author

erikasby commented Aug 7, 2023

Not really sure, from my side everything is smooth and works fine, I don't get any errors regarding of undefined reading name.

Also I tested the the specific conditions, for example if discourse was chosen as category and it appears in the language and curriculum_slug list it then appears as an added slug to "View curriculum".

The issue with slugs should be fine really, the slugs from curriculum are same as in forum's settings file and the forum's slugs are taken from the Discourse itself - that's why container, controller and category variables are needed in the first place.

Will try to check on Thursday again if you won't be able to or wouldn't have time to find anything new.

@erikasby
Copy link
Contributor Author

It seems that indeed I needed to use .toLowerCase() to have proper comparisons. I tested it and it works on my end if the category starts with an uppercase letter.

@ahmaxed
Copy link
Member

ahmaxed commented Aug 15, 2023

Thanks for the iterating on this pr, I am still getting the following error:
Cannot read properties of undefined (reading 'name')
I will be working on the theme next week and will prioritize getting your pr in.
Could you please share the setup you are using to create and test the theme with me?

@ahmaxed
Copy link
Member

ahmaxed commented May 23, 2024

@ShaunSHamilton, could you take a look into this?

@ShaunSHamilton
Copy link
Member

@erikasby Thanks for working on this. I was not allowed to push to your main branch. So, I created a new PR.

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.

forum's curriculum nav item should link to relevant language of the curriculum
4 participants