-
Notifications
You must be signed in to change notification settings - Fork 643
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
Refactoring - Easier rebranding of the site #1700
Conversation
…d the ContentPagesController and make it serve up the Home page. Make Policies pages served by ContentService, and try to fix what I think is a race condition due to using MarkDownProcesser in a non-thread-safe way. Fix ContentService handling of html files.
…Menu. And Rework the /Error pages (except the oh now we broke something) to be provided using an ErrorsController so that we don't need a separate ErrorsLayout.
…by adding overrides in the branding folders.
This fixes #1701. Just so we can say it fixes something. |
@@ -126,7 +126,7 @@ | |||
var ver = ApplicationVersionHelper.GetVersion(); | |||
|
|||
<p id="releaseTag"> | |||
This is the NuGet Gallery. | |||
This is the @(ConfigurationManager.AppSettings["Gallery.Brand"].Trim()). |
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 only reads from web.config, not CSCFG. Can you add "Brand" as a string property to IAppConfiguration? Then it will be autopopulated and will be available in ConfigurationService.Current (you can use Container.Kernel.Get<...> in the ViewHelpers since we don't unit test them anyway)
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.
Sure, I'll do it that way.
} | ||
else if (fileName.EndsWith(".md")) | ||
{ | ||
lock (MarkdownProcessor) |
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.
If we're worried about multi-threading here, let's just construct a Markdown object inside the method. It's a pretty low-impact operation.
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.
Markdown was indeed not thread-safe, and I encountered a race issue here. Local variable should be fine if its lightweight, I had assumed there was a reason for the staticness.
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.
Preemptive micro-optimization on my part. Bad Andrew! NO!
Note: You need to add the setting to BOTH the CSDEF and CSCFG files in the NuGetGallery.Cloud project in order for them to be usable in CSCFG. Unfortunately you have to pre-define all the settings you're going to use. |
This is a refactoring to make rebranding easier, there should be no functional changes to the site visible from this - APART from one thing. You now need to specify the Gallery.Brand setting in your web.config.
Extra Deployment steps: