freeCodeCamp / forum-theme

Themes that we use on our forums
BSD 3-Clause "New" or "Revised" License
14 stars 30 forks source link

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

Closed erikasby closed 5 months ago

erikasby commented 1 year ago

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

ahmaxed commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 5 months ago

@ShaunSHamilton, could you take a look into this?

ShaunSHamilton commented 5 months ago

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