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

Feat: Already uploaded images(for edit purposes) #130

Merged
merged 4 commits into from
Sep 20, 2017
Merged

Feat: Already uploaded images(for edit purposes) #130

merged 4 commits into from
Sep 20, 2017

Conversation

sabrio
Copy link
Contributor

@sabrio sabrio commented Sep 15, 2017

Feature requested in #12 for edit purposes, also I added demo on the first page.
Please review this PR and let me know what do you think. I/We need this feature immediately in.
Thank you.

@UncleDave UncleDave added this to the v1.0.0 milestone Sep 15, 2017
@UncleDave
Copy link
Collaborator

Looks good - thanks for this PR and the demo, could you please also update the readme accordingly?

@sabrio
Copy link
Contributor Author

sabrio commented Sep 16, 2017

NP @UncleDave, yes ofc I can update it.

Copy link
Owner

@aberezkin aberezkin left a comment

Choose a reason for hiding this comment

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

Nice! Can you please add some spaces if(..) -> if (...) to match our code style?


images = [];

onRemoved(event){
Copy link
Owner

Choose a reason for hiding this comment

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

){ => ) { also :)

@aberezkin
Copy link
Owner

Also, I'm glad that you chose our lib for your first GitHub contribution :)

@UncleDave
Copy link
Collaborator

On the subject of code style, as raised by @aberezkin, there is an .editorconfig file in the repo - WebStorm supports this natively, and Visual Studio Code has a plugin for this although I'm not sure Visual Studio Code will match all of our styles correctly. If you're not using WebStorm I don't mind giving it a run through the autoformat after we merge this.

@sabrio
Copy link
Contributor Author

sabrio commented Sep 18, 2017

@aberezkin thank you for your review&code style correction and I am also glad to help to improve this library because I found it really useful and simple. This is what Autoformat did to the component 2a82743. I will revert it if needed. Let me know what do you think?

@UncleDave
Copy link
Collaborator

Fine by me.

@sabrio
Copy link
Contributor Author

sabrio commented Sep 20, 2017

@UncleDave when this feature is going to be available for use in the package? Are you planning for pre-release so can use the development features and test them? smth like: 1.0.0-alpha.1

@UncleDave
Copy link
Collaborator

I have made that suggestion in #102, but I'm not the one who controls npm releases.

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

Successfully merging this pull request may close these issues.

None yet

3 participants