-
Notifications
You must be signed in to change notification settings - Fork 20
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(cc-domain-management): allow newly added domain to be marked as primary #1214
fix(cc-domain-management): allow newly added domain to be marked as primary #1214
Conversation
404575d
to
b75322f
Compare
const hasPathPrefix = pathPrefix != null && pathPrefix !== '/'; | ||
// TODO: once the API returns actual ids, this should be adapted (either refetch or use the API response) | ||
const id = hasPathPrefix ? hostname + pathPrefix : hostname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot remember why I did that in the first place :/ I do remember that there were conflicts with some API routes not handling the trailing slash properly but it seems to work fine without it and usually I leave a comment to explain why I did something weird so it may be something I forgot to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue was as follows:
- when adding a new domain, I used to not send trailing slashes to the API because that's what was done before,
- so the
id
was thehostname
without the path prefix if the path prefix was just "/", - but at some point I decided to send trailing slashes to the API when adding a domain and I forgot to update the
id
here to reflect that.
b75322f
to
68c3961
Compare
🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-domain-management/fix-newly-added-domain-as-primary/index.html. This preview will be deleted once this PR is closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the code, I haven't tested the demo.
🔎 The preview has been automatically deleted. |
Fixes #1213
What does this PR do?
How to review?
localhost:6006/demo-smart/
,cc-domain-management
,app-node
: