Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

Theme management #210

Merged
merged 3 commits into from
Aug 23, 2016
Merged

Theme management #210

merged 3 commits into from
Aug 23, 2016

Conversation

kevinansfield
Copy link
Collaborator

@kevinansfield kevinansfield commented Aug 17, 2016

refs TryGhost/Ghost#7204, requires TryGhost/Ghost#7209

  • replaces theme dropdown with a table
  • adds theme upload modal
    • validates theme mime type
    • prevents upload of casper.zip (default Casper theme can't be overwritten)
    • warns if an upload will overwrite an existing theme
    • gives option of immediately activating the uploaded theme or closing after successful upload
  • adds theme activation link/action
  • adds theme download link/action
  • adds theme deletion modal
    • warns about no undo possibility
    • offers possibility to download theme
  • modifies mirage config to handle theme changes

TODO:

  • replace themes dropdown with table
  • upload theme modal
  • activate action
  • delete action
  • download action
  • tests
  • styling
  • 🐛 flash of error state between upload finished and success screen
  • validate file mime-type/extension after file selection
  • warn about theme overwrite after file selection
  • don't give "Activate Now" option if uploaded theme is already active
  • handle validation errors when deleting
  • add "casper.zip" client-side validation (it's not possible to overwrite default casper)
  • display folder name next to themes if there are package.json name duplicates

@kevinansfield kevinansfield force-pushed the theme-uploads branch 2 times, most recently from 3c4276b to dd59d3e Compare August 17, 2016 15:45
@kevinansfield kevinansfield changed the title Theme uploads [WIP] Theme uploads Aug 18, 2016
@kevinansfield kevinansfield force-pushed the theme-uploads branch 2 times, most recently from 69f2f01 to 52acf2c Compare August 19, 2016 07:07
@kevinansfield
Copy link
Collaborator Author

@acburdine would be good to get a second pair of eyes on these changes: 111bc33

I've had to introduce a global event bus in order to have parent->child component communication. I tried doing this by registering the child with the parent but that ran into problems because:

  1. the file upload component is destroyed when switching to the "warning" state
  2. the file input itself is locked down by browsers and so it's not possible to trigger things programmatically

@kevinansfield
Copy link
Collaborator Author

Possible alternatives that spring to mind:

  1. changing the gh-file-uploader component so that the file property can be passed in, we could then use didReceiveAttrs to automatically trigger the upload (writing this that actually sounds like a better approach?)
  2. subclassing the gh-file-uploader to better cater for the theme uploader's requirements

@acburdine
Copy link
Member

@kevinansfield 👍 will look in a bit

@kevinansfield
Copy link
Collaborator Author

Third option: don't swap out the upload component but instead change the style to display: hidden and abs-position the warning over the top - bonus would be that the modal doesn't change height but we're still left with the tight coupling.

I think for now I'm going to go with the first alternative option of modifying the uploader component to accept a file attribute - I think that's the simplest option for now without introducing additional concepts such as the event bus (although I do think the event bus concept would be useful, e.g. for handling global keyboard shortcuts inside components, and avoiding some of the complexity involved in getting save buttons to spin when saving is happening outside of it's own promise).

However I think my main takeaway is that the uploader components are currently encapsulating too much behaviour, things like this would be a lot easier if they were split into more composable components, e.g.:

  • "uploader" wrapper component that handles the actual file upload and validation state
  • "progress" component that displays the progress bar
  • "file-input" component that has a button/dropzone for obtaining a file and passing it to the uploader
  • upload-error" component for displaying the error state

@kevinansfield kevinansfield force-pushed the theme-uploads branch 3 times, most recently from 509821e to 82cafce Compare August 23, 2016 08:45
no issue
- upload components will now trigger a passed-in `fileSelected` action upon file selection - useful when users of the components want to utilise the file object without supplying a custom validation action
refs TryGhost/Ghost#7204, requires TryGhost/Ghost#7209

- replaces theme dropdown with a table
- adds theme upload modal
    - validates theme mime type
    - prevents upload of `casper.zip` (default Casper theme can't be overwritten)
    - warns if an upload will overwrite an existing theme
    - gives option of immediately activating the uploaded theme or closing after successful upload
- adds theme activation link/action
- adds theme download link/action
- adds theme deletion modal
    - warns about no undo possibility
    - offers possibility to download theme
- modifies mirage config to handle theme changes
@kevinansfield
Copy link
Collaborator Author

I've squashed ready for merge + release testing.

Outstanding TODOs:

  • finish acceptance tests (~90% complete)
  • display the folder name alongside package name when there are duplicate package names

Note that due to time constraints I've left the event bus system discussed above in place for now. I have some refactoring planned for the upload components so I can look at removing the event bus service at that point if it doesn't prove useful in any other circumstances.

@kevinansfield kevinansfield changed the title [WIP] Theme uploads Theme management Aug 23, 2016
@acburdine acburdine merged commit a14ac50 into TryGhost:master Aug 23, 2016
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this pull request Aug 23, 2016
refs TryGhost#210
- adds missing acceptance tests for theme deletion
- adds theme deletion endpoint to mirage config
- fixes mirage settings update endpoint (was previously removing config that the client didn't send and also losing the `type` key for all entries preventing the `GET` request from working properly)
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this pull request Aug 23, 2016
refs TryGhost#210
- removes unused `activeTheme` property on `gh-theme-table`
- updates label generation in `gh-theme-table` to add folder names when there are duplicate package.json name+version combos
@kevinansfield kevinansfield deleted the theme-uploads branch February 4, 2017 10:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants