search

okkur / syna

Highly customizable open source theme for Hugo based static websites
https://syna.okkur.org/demo/
Apache License 2.0
250 stars 133 forks source link

Fix rel lang fragment name issue #801

Closed mpourismaiel closed 4 years ago

mpourismaiel commented 4 years ago

What this PR does / why we need it: Fragment names didn't omit language name which was causing JS selectors to fail. This PR renames <fragment>.<lang> to <fragment>.

Which issue this PR fixes: fixes #798 fixes #799

Release note:

- Fix scripts not working in multilingual websites properly
mpourismaiel commented 4 years ago

Okay this was really hard and a lot of changes were required. I tried to implement it in a way that wouldn't require us to change it later on. An id was added to each fragment to ensure filenames are valid in multilingual and subitems needed their names changed to make sure they work with multilingual as well.

stp-ip commented 4 years ago

Double checking. Is #799 fixed with this PR?

Bigger change so have to dive in a bit deeper. Thanks for hanging in there until I get the review done :)

mpourismaiel commented 4 years ago

This fixes #799 as well. Also totally fine on waiting for the review, it's a big change.

stp-ip commented 4 years ago

Any specifics on the failing netlify test?

Edit: all good.

mpourismaiel commented 4 years ago

I use scratch whenever a value needs to be updated further down the code. I don't like how Go templates treats variables and scratch provides a more familiar approach. I can change it to normal variables if you prefer it.

Will add the comment to describe the reasoning.

stp-ip commented 4 years ago

Ok with scratch for now as we are using it for quite some time. Wouldn't be opposed now that Go actually has working variables to remove scratch in the long run. Anyway one thing that makes this harder to understand is the naming. "tmp_" doesn't add much information.

mpourismaiel commented 4 years ago

Agreed on removing as much scratch as possible. I use "tmp_" to make sure the variable I'm creating will not be used outside of it's scope. All other scratches are created with other usages in mind. For example "fragmentdirectories" is not used outside of fragments.html file but it's there for anyone who wants access to it. But any scratch that has "tmp" prefix should not be used anywhere else. It's an undocumented code style I use :D

stp-ip commented 4 years ago

Happy with the variable usage, if it gets documented close to the code ;)

mpourismaiel commented 4 years ago

Happy with the variable usage, if it gets documented close to the code ;)

I have to update the contribution guide lines and include it in there. Is it okay to do so in another issue and PR?

stp-ip commented 4 years ago

Should probably also be added as at the top of the file as it's mainly used within the already complicated fragment.html or?

Good to do this in a follow on PR.

mpourismaiel commented 4 years ago

I'll add it in another PR and will create an issue tracking our contribution guideline.