Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generic Site settings Deployment Step #6570
Generic Site settings Deployment Step #6570
Changes from 5 commits
e091e6e
c980d97
6a1ac45
6793135
5d8c71f
c2bfa2f
11785f1
0cdde48
ecb254e
000b884
f74aca6
818ccbc
3c42aca
3230174
aacdec3
187f706
03d4270
b3337b1
0978700
3040d64
938d45f
0465477
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Remove this reference
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.
We need to test here we don't get double encoding. Should be ok, but just put an accent in one of the localized values, and check the page source after to make sure the encoding is correct
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.
Tested is all good
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.
this needs to go in a separate startup with
RequiresFeatures
just for the default login settingsThere 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.
ok, please check.
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.
We should find the property if it exists already, so it is only one step that is being added.
I will do this, while I am checking the registration with both attributes (i.e.
Feature
andRequiresFeature
works ok)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.
@agriffard have changed it to look for an existings
Settings
property rather than creating a new one every time.output changes from
An optimization, and makes it clearer that all the steps are contained in one
If you are happy with the change let me know, and I think we can merge it.
I've checked the
Feature
andRequiresFeature
are working fine together. i.e. if you disable deployment but still have registration the startup doesn't fire.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.
Great