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

Tagging #293

Merged
merged 3 commits into from
Apr 9, 2015
Merged

Tagging #293

merged 3 commits into from
Apr 9, 2015

Conversation

sonalkr132
Copy link
Collaborator

I added acts-as-taggable-on to issues. Surprisingly enough I passed rspec. I have added sorting to tags as well.

  • Do projects need to be taggable too?
  • How far should we take the styling of tag? We can get by changing delimiter of tags from comma to something else and autocomplete of jquery or do we need to something like chosen-rails or select2-rails?

#In_progress #Needs_feedback

@sarupbanskota
Copy link
Contributor

@sonalkr132 let projects not be taggable for now.

about the styling, let's stick with comma + autocomplete. we can revisit when we want to improvise :)

sarupbanskota added a commit that referenced this pull request Apr 9, 2015
@sarupbanskota sarupbanskota merged commit 670f4ce into glittergallery:master Apr 9, 2015
@Alwahsh
Copy link
Collaborator

Alwahsh commented Apr 9, 2015

Surprisingly enough I passed rspec

Why is that surprising? The issues part is not covered by any tests yet since #292 is not yet merged and you didn't add any tests so there's no way you can fail rspec :)

I see you've worked on some of the stuff I worked on too in #292 ... So this would close #285

Also, a question... if you've defined a list of accepted tags, wouldn't it make more sense to make the user select one of them instead of leaving him a textbox where he writes any tag he wants... I tried this locally and I couldn't add any issue and never got a clue on why's that except till I saw the line where you force the tag to be one of (bug feature improvement inspiration).. This would be a really bad experience for users since they can't magically guess which tags you hardcoded in the code.

@Alwahsh
Copy link
Collaborator

Alwahsh commented Apr 9, 2015

@sarupbanskota I'm not sure if my above comment would be an enough reason, but I think this should be reverted at least till the issue I mentioned above is fixed and till we have some tests?

@rohitpaulk
Copy link
Member

I'm creating a PR to revert this - We really need tests for all this functionality.

@rohitpaulk rohitpaulk mentioned this pull request Apr 9, 2015
@sarupbanskota
Copy link
Contributor

Yep I agree - thanks for reverting it Paul.

On Thursday, April 9, 2015, Rohit Paul Kuruvilla [email protected]
wrote:

I'm reverting this - We really need tests for all this functionality.


Reply to this email directly or view it on GitHub
#293 (comment)
.

Sarup

@sonalkr132
Copy link
Collaborator Author

hey! why would you merge this in first place. I need feedback, I had work more on this and thats why:

#In_progress #Needs_feedback

I haven't added the style or autocomplete. I hadn't written any tests. As @Alwahsh pointed out, I wasn't sure if user can make any tag they want or we should have limits and I added a limited version because in comments it said TODO: find 5 most popular tags among designers.

Surprisingly enough I passed rspec.

I meant even after such a big change I passed rspec which is surprising and something is obviously wrong and we need to fix it. ie write tests.

@Alwahsh
Copy link
Collaborator

Alwahsh commented Apr 9, 2015

@sonalkr132 I think it's better to indicate in the title when your PR is in progress.
Also... The number of PRs that you have in progress is increasing... Why not focus on one thing(or part of the app) till it's complete? you're just making it harder -for yourself and for me- by giving more chances of conflicts like that... If you had opened this PR before I opened #292 , you'd have blamed me for working on issues.

I meant even after such a big change I passed rspec which is surprising and something is obviously wrong and we need to fix it. ie write tests.

yup.. We have no tests for the issues part as I mentioned and fixed in #292.

@sonalkr132
Copy link
Collaborator Author

@Alwahsh

it's better to indicate in the title when your PR is in progress.

Yeah, thats a nice idea.

Why not focus on one thing till it's complete?

I leave them in progress cause I am waiting feedback on them. Out of 7, 2 are in progress so not that many I would say.

We have no tests for the issues part as I mentioned and fixed in #292.

My finals are close -_- I can't work much. May you can pull this my branch on issues tagging? I will add auto complete and some style to tags. Or may if I have time, I will pull from yours. Don't use rebase or force update if I have pulled.

"Should the list be decided by one single admin or users should be able to add whatever tag they want on tags?" is still unanswered.

@Alwahsh
Copy link
Collaborator

Alwahsh commented Apr 9, 2015

May you can pull this my branch on issues tagging? I will add auto complete and some style to tags. Or may if I have time, I will pull from yours. Don't use rebase or force update if I have pulled.

I didn't really understand this paragraph fully but it would be easier for me to just add the gem myself and write the tests as I introduce the feature. Issues tagging is something that I planned to work on but not at this point. So, I prefer to put my effort in finishing the partly implemented but not yet fully working features before I introduce others for now.

My finals are close -_- I can't work much.

Yeah... I can feel you... While I still have some time before my finals but I have things to do in my college too (projects and practical exams :( ). It'd have been cool if they gave bonus for open source contribution :)

"Should the list be decided by one single admin or users should be able to add whatever tag they want on tags?" is still unanswered.

Let me tell you my opinion... I believe whatever tags is better but one still gets a list of previously created tags as suggestions as he's writing. If he finds a suitable tag, he selects it. Otherwise, creates his own. Not sure how others would think of that though.

@sonalkr132
Copy link
Collaborator Author

I didn't really understand this paragraph fully

Sorry, I meant that either of us can pull from branch of other and they can just sent one PR. About adding tags.. I only started it because I didn't find anything else to work on. I think I I will work on this after your PR is merged.

Otherwise, creates his own.

Wouldn't it require that someone moderates tags? List of tags can be as extensive as it has to be. We can even write a wiki page about tagging. I think thats how it works on github?

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

Successfully merging this pull request may close these issues.

4 participants