-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Tag static files from a theme-gem with a sticker #406
Conversation
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.
Good addition!
src/components/FilePreview.js
Outdated
@@ -32,9 +32,18 @@ export default class FilePreview extends Component { | |||
} | |||
return ( | |||
<div className="file-preview"> | |||
{ |
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 know I've put a few inline conditions in the front-end but it's better to pull them out of return statement which makes it more readable.
let theme_indicator;
let btn_delete;
if(file.from_theme) {
theme_indicator = <div...
} else {
btn_delete = <button...
}
src/components/FilePreview.js
Outdated
<button onClick={() => this.handleClickDelete(file.path)} className="delete" title="Delete file">x</button> | ||
{ | ||
!file.from_theme && | ||
<button onClick={() => this.handleClickDelete(file.path)} className="delete" title="Delete file">x</button> |
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.
Why should we not allow deleting theme files?
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.
Why should we not allow deleting theme files?
While I was greatly surprised that you asked this question, on reflecting further, I'm assuming you were talking about removing the Jekyll::StaticFile
object from the site_payload
rather than deleting the file proper..
To answer otherwise, three things to consider:
- The theme-gem is supposed to be a read-only object for Jekyll.
- We have no write-access outside
site.source
. - When users click
delete
, on a theme-file they're sending adelete
request to therelative_path
of the static file which is sanitized to point to a non-existing entity in thesource_dir
. (e.g.<theme>/assets/logo.png
will resolve as<site.source>/assets/logo.png
which doesn't exist. Otherwise, Jekyll would've read that file in to memory instead)
There's no valid use-case to allow write-access to the theme-gem in future either
Currently, there is no way to easily distinguish which static-file displayed in
Dropzone
is from thesite.source
and which one is from the theme-gem.Additionally, there's no sense that users can click delete on a static-file from the theme-gem.
This PR resolves both of above.