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

Allow the user to disable the use of the cdnjs #17

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

eyedeekay
Copy link

I wanted to host this pastebin as an I2P service and discovered there wasn't a way to disable the use of the CDN as the source of the CSS and Javascript. I wanted to at least be able to use the defaults without the CDN, so today(at last) I wrote my first ~95 lines of Rust. I added a new variable(themepath) which can either point to a CDN source or be left blank to use the default theme without needing another service. Looking forward to your feedback.

@raftario
Copy link
Owner

raftario commented Mar 8, 2020

Thanks for the PR, that's definitely a nice feature to have. I see a bit of duplicate code that could be merged into functions but apart from that it looks good. I'm a bit busy right now, will review it as soon as I have more time, probably some time in the next week.

Copy link
Owner

@raftario raftario left a comment

Choose a reason for hiding this comment

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

Pretty good for a first time ! There's still a couple things that I'd like to be changed, I can help with those if necessary. In the future, also avoid changing the lockfile in PRs unless the purpose is to fix bugs or security issues in dependencies.

LICENSE Outdated Show resolved Hide resolved
src/routes.rs Outdated Show resolved Hide resolved
src/routes.rs Outdated Show resolved Hide resolved
Comment on lines +260 to 321
/// CSS file to style the page from a local source
pub async fn css(request: HttpRequest, identity: Identity) -> impl Responder {
if let Err(response) = auth(identity, request).await {
return response;
}

let contents = {
#[cfg(feature = "dev")]
{
fs::read_to_string(&*CSS_PATH).expect("Can't read spectre.min.css")
}
#[cfg(not(feature = "dev"))]
{
CSS_CONTENTS.to_owned()
}
};
HttpResponse::Ok()
.header("Content-Type", "text/css")
.body(contents)
}

/// Icon-specific CSS file to style the page from a local source
pub async fn icon(request: HttpRequest, identity: Identity) -> impl Responder {
if let Err(response) = auth(identity, request).await {
return response;
}

let contents = {
#[cfg(feature = "dev")]
{
fs::read_to_string(&*ICON_PATH).expect("Can't read spectre.min.css")
}
#[cfg(not(feature = "dev"))]
{
ICON_CONTENTS.to_owned()
}
};
HttpResponse::Ok()
.header("Content-Type", "text/css")
.body(contents)
}

/// JS file to highlight code from a local source
pub async fn js(request: HttpRequest, identity: Identity) -> impl Responder {
if let Err(response) = auth(identity, request).await {
return response;
}

let contents = {
#[cfg(feature = "dev")]
{
fs::read_to_string(&*JS_PATH).expect("Can't read spectre.min.css")
}
#[cfg(not(feature = "dev"))]
{
JS_CONTENTS.to_owned()
}
};
HttpResponse::Ok()
.header("Content-Type", "text/javascript")
.body(contents)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, a lot of duplicate code that could be merged, however that one is trickier since different values are used depending on whether or not the dev feature is enabled. Cleanest way to do it would probably be a macro, I can take care of that since you're new to Rust.

src/setup.rs Outdated
@@ -120,6 +122,7 @@ impl Default for Config {
impl Default for HighlightConfig {
fn default() -> Self {
Self {
themepath: "https://cdnjs.cloudflare.com/ajax/libs/spectre.css/0.5.8/".to_owned(),
Copy link
Owner

Choose a reason for hiding this comment

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

Oops !

@raftario raftario self-assigned this Mar 9, 2020
@raftario
Copy link
Owner

Hey, sorry for not following up with that but this whole situation has been quite something and made me forget about this PR. I plan to rewrite filite completely in the next few days and get rid of the CDN altogether. I'll keep this open until the rewrite is done, but I'll probably close it afterwards.

@eyedeekay
Copy link
Author

Thanks for getting back to me, glad to hear that the feature I wanted will be available in any case. Looking forward to updating.

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