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(routeselector/resourceroom): remove accidental comments + fix typo #1859

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Mar 20, 2024

Problem

merged previous PR too early, had a commit that was unpushed to upstream that removes extra comments and fixes a typo.

Solution

  1. fix typo
  2. remove extra comment
  3. validate on terms-of-use
  4. use decoded params instead

Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

Hey I think this PR breaks functionality for a lot of old pages which were pre-cms - in the past all pages were specified in slugified form (e.g. blah-blah.md, see this repo for examples). Would it be possible to add an exclusion for dashes?

@seaerchin
Copy link
Contributor Author

Hey I think this PR breaks functionality for a lot of old pages which were pre-cms - in the past all pages were specified in slugified form (e.g. blah-blah.md, see this repo for examples). Would it be possible to add an exclusion for dashes?

good catch, added a special regex for this

Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

lgtm

return !specialCharactersRegexTest.test(encodedName)
const decodedName = decodeURIComponent(encodedName)
return (
value === "terms-of-use.md" ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this custom check for terms-of-use now

Comment on lines 72 to 73
"/sites/:siteName([a-zA-Z0-9-]+)/folders/:collectionName([a-zA-Z0-9-]+)/subfolders/:subCollectionName/editPage/:fileName",
"/sites/:siteName([a-zA-Z0-9-]+)/folders/:collectionName([a-zA-Z0-9-]+)/editPage/:fileName",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, do we not need to check subCollectionName?

@seaerchin seaerchin merged commit f3dc16c into develop Mar 21, 2024
8 checks passed
@seaerchin seaerchin deleted the chore/add-validation branch March 21, 2024 05:15
@seaerchin seaerchin mentioned this pull request Mar 21, 2024
7 tasks
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.

2 participants