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

Add multiple files per snippet #43

Merged
merged 32 commits into from May 31, 2019

Conversation

GWillmann
Copy link
Contributor

What an awesome project Snibox is! I hope my contribution make it even better.

This PR adds support of multiple files per snippet.
We needed this kind of feature at Kinoba and I saw that several people wanted it also

I added the following model: SnippetFile which belong_to a Snippet.
Also I was inspired by GitHub's snippet system and I replace the Snippet title by a description.

I also changed the tests according to the new structure and added new ones:

  • Add a snippet file
  • Remove a snippet file

I am not extremely happy with the design I came up when one edits a Snippet, if you guys have some suggestions I'd be happy to change it.

@GWillmann
Copy link
Contributor Author

Hello @vavgustov,

Have you had the time to review my PR?

Let me know if I can improve something :)

@vavgustov
Copy link
Member

hi @GWillmann ,

I apologize for the delay, I have very limited time now. I'll check your PR at 04/21.

@vavgustov
Copy link
Member

vavgustov commented Apr 22, 2019

hi,

just did a quick look at your PR. Great work! :godmode:

Some quick recommendations about UI and other things that I noticed:

  1. Current second column from existing UI not optimized for long descriptions.
    I think we need both title + description for snippet.
    That means 20190401062316_convert_snippets_to_snippet_files.rb shouldn't remove title field from snippets table.
    Title should be required and description should be optional.

  2. 'Edit mode': 'Add snippet file' => should be just one button (at this moment this button under each file). Name should be 'Add file'.

  3. Files probably should be collapsible?

  4. 'Show mode': files shouldn't have 'delete' button.

  5. 'Edit mode': labels should be moved above files.

  6. 'Edit mode': file textfield and other UI elements should be moved to header? Delete button should stick to textfield as icon?

Probably a lot of other small UI improvements :)

I didn't had enough time so I just did a quick draft for some changes for 'show' mode:
show_mode

PS this is not final recommendations. I need more time to check this big PR in more detail and then
provide detailed suggestions/screenshots at 04/28 or 05/05. Sorry for the delays and thanks for your patience!

- 1. Readd title attribute to Snippet model and makes description
optional
- 2. `Add snippet file` button was renamed to `Add file` and only one occurence of the button was kept
- 3. Snippet files are now callapsible in show and edit mode
- 4. Remove `delete` button in show mode for snippet files
- 5. Move labels above snippet files in create/edit mode

Adds tests for snippet files callapsible capability
@GWillmann
Copy link
Contributor Author

Hi @vavgustov,

thanks for your feedback!

I have taken them into account in my PR. Quick info:

Commit 98779e6

Removes chromedriver-helper gem because it is now deprecated in favor of the webdrivers gem

Commit bf4d058

  1. I readded the title attribute to the Snippet model and made the description attribute optional
  2. Add snippet file button was renamed to Add file and only one occurence of the button was kept
  3. Snippet files are now callapsible in show and edit mode
  4. I removed the delete button in show mode for snippet files
  5. I moved labels above the snippet files in create/edit mode

I also added tests for snippet files callapsible capability

Finally I am not sure I understood correctly your comment:

6 - 'Edit mode': file textfield and other UI elements should be moved to header? Delete button should stick to textfield as icon?

Could you rephrase it please?

@vavgustov
Copy link
Member

awesome, thanks!

don't worry about 6, I'll take care of it.

also I'll make some experiments with existing UI soon and commit them directly to your PR (few small changes already added).

@vavgustov
Copy link
Member

vavgustov commented May 3, 2019

@GWillmann can you replace underscore_case to camelCase for js code when you have a time ?

e.g. snippet_files should be snippetFiles
or destroy_snippet_file should be destroySnippetFile

Exception: requests to backend, e.g. it's ok to have:
snippet_files_attributes: snippetFilesAttributes,
because we use underscore_case for model properties (rails).

for ActiveModel::Serializer response you can use something like this: https://stackoverflow.com/a/40618505 so the server can respond directly with snippetFiles instead of snippet_files.

PS don't forget to pull the changes for this PR :)

@GWillmann
Copy link
Contributor Author

GWillmann commented May 7, 2019

Thanks for your feedbacks @vavgustov!

I have changed made the changes you requested.

Do you think you will be able to merge this PR soon? 🙂

@vavgustov
Copy link
Member

thanks!
It will be merged during May.

@vavgustov vavgustov mentioned this pull request May 28, 2019
8 tasks
@vavgustov vavgustov merged commit b19949a into snibox:master May 31, 2019
@vavgustov
Copy link
Member

ok I added some changes and merged to master. I would like to make more but don't have a time as usual 😃. Thanks for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants