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 upload of only image file types in import-image module #440

Closed
wants to merge 6 commits into from
Closed

Allow upload of only image file types in import-image module #440

wants to merge 6 commits into from

Conversation

tsparksh
Copy link
Member

Fixes #429
image

@tsparksh
Copy link
Member Author

@Rishabh570 @aashna27 @vikul1234 please review

@aashna27
Copy link

@thesparks great work! but there are still some conflicts, also the 3 image file types you mentioned aren't the only image types.

@tsparksh
Copy link
Member Author

@aashna27 what types of image you need? All image/* mime types?

@tsparksh
Copy link
Member Author

Task has only tag html. Maybe js check is not needed at all?

@Rishabh570
Copy link

@thesparks Yep, the task has HTML tag only...you don't need to worry about server-side validation of the file uploaded for this task...just accept=image/* thing would make it?
@aashna27 Thoughts?

@aashna27
Copy link

Yeah! true js won't be needed. 😅

@tsparksh
Copy link
Member Author

@Rishabh570 , @aashna27 fixed) please confirm CGI task

@Rishabh570
Copy link

@thesparks One more thing, make sure your commits are related to this PR only...I see a commit containing GIF related code...and you should squash your changes into a single commit (for PR containing small changes) to make the commit history as clean as possible...

@tsparksh
Copy link
Member Author

tsparksh commented Oct 24, 2018

just accept=image/*

image/* didn't work on Linux, so i added js, my mistake

@thesparks One more thing, make sure your commits are related to this PR only...I see a commit containing GIF related code...and you should squash your changes into a single commit (for PR containing small changes) to make the commit history as clean as possible...

@Rishabh570, Sorry, I thought I could merge commits after sending

@aashna27
Copy link

The "image/* " would work, see while uploading file , the file manager would no longer show "All files."
you can merge the commits you have pushed. and remove the ones not required.
https://help.github.com/articles/about-git-rebase/ this would help you to learn how to merge commits.

@tsparksh
Copy link
Member Author

tsparksh commented Oct 24, 2018

@aashna27,

The "image/* " would work,

This works at on windows computer, but not on Linux notebook (maybe its bug of deepin file manager):
image

To squash and merge pull requests, you must have write permissions in the repository, and the repository must allow squash merging.

I can't change anything here, but I can do it before future commits, right?

@aashna27
Copy link

Okay, i ll look into it. Till then you can work on other issues.

@aashna27
Copy link

You need to squash the multiple commits that have been made into 1.

@tsparksh
Copy link
Member Author

tsparksh commented Oct 24, 2018

@aashna27, but i cant get new CGI task until pass the last

@aashna27
Copy link

@thesparks as i mentioned please squash your commits its difficult to review multiple commits, can't keep track. https://help.github.com/articles/about-git-rebase/ this would help you learn the commands to merge the commits. 😄
I really appreciate your enthusiasm and work 👍

@tsparksh
Copy link
Member Author

tsparksh commented Oct 24, 2018

i dont understand how it works...

@handlerug
Copy link

+1. It will trigger some strange merge conflicts
screenshot 2018-10-24 at 23 53 05

@Rishabh570
Copy link

@thesparks do git rebase -i it will open a list of recent commits, then replace the word pick with s (in front of the commit message you wanna squash) to squash that commit into its previous commit...Here is the example GIF

squash_commit

Here I did git rebase -i main (because I was on different branch than main)

Hope this helps, ask if you still have queries...!!!

@tsparksh
Copy link
Member Author

deepin-screen-recorder_select area_20181025001803
why noop?

@Rishabh570
Copy link

You need to provide a commit hash, eg. git rebase -i {seven_characters_hash} in order to do interactive rebase after that commit or you can do something like git rebase -i HEAD~n where n is number of commits you wanna do rebase onto...(like if you have 6 commits in your PR, so you might wanna do git rebase -i HEAD~6)
Some explanation here

@handlerug
Copy link

git rebase -i HEAD~4 or git rebase 1935967 shows nearly 16 commits, and when I pick or squash that commits, git says to resolve merge conflicts. So I've looked into Sourcetree and there are 3 branches, one of them has @thesparks commits. How to rebase only main branch?

@avsingh999 avsingh999 added the gci-candidate Issue being considered for gci label Oct 25, 2018
Copy link
Member

@tech4GT tech4GT left a comment

Choose a reason for hiding this comment

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

@thesparks @aashna27 @Rishabh570 Is something wrong here, I don't see any code changes, just elimination of blank lines?

@tech4GT
Copy link
Member

tech4GT commented Oct 25, 2018

Also don't worry about squashing commits, we would be merging all the changes done in one pull request as one commit only.

@tsparksh
Copy link
Member Author

tsparksh commented Oct 25, 2018

@tech4GT I tried to merge commits, and made two extra commits and something went wrong :/
I changed

<input type="file" id="fileInput" value="">

to

<input type="file" id="fileInput" value="" accept="image/*">

Also I added a validation with js, but it was redundant and I deleted it.

@tech4GT
Copy link
Member

tech4GT commented Oct 25, 2018

Hmm, I see...
A piece of advice for you, never make changes directly to your main branch, make a new branch for the changes you make and then open a pull request from there. As for this, I'll try and fix this. Thanks!

@tech4GT
Copy link
Member

tech4GT commented Oct 25, 2018

Hmm @thesparks I figured out what happened here, nothing went wrong, you merged the branches correctly but the line you added is already present in the code, you did not see it because you did not pull the latest changes.
Is this your gci task?

@tsparksh
Copy link
Member Author

@tech4GT, yes its gci task

@tech4GT
Copy link
Member

tech4GT commented Oct 25, 2018

Since this one is already fixed in the code but you made the right efforts for it, you can move on to the next task and I'll make sure you get the credit for this one. Thanks! 😄
cc @jywarren

@tech4GT tech4GT closed this Oct 25, 2018
@tsparksh
Copy link
Member Author

Since this one is already fixed in the code
you did not see it because you did not pull the latest changes.

strange i did everything through githab.com (1935967)

@tech4GT
Copy link
Member

tech4GT commented Oct 25, 2018

These things happen sometimes, don't worry about it. Thanks a lot for your patience😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gci-candidate Issue being considered for gci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants