Apache Allura™

Forge software for hosting software projects

Brought to you by: alexluberg, brondsem, ccruz, deshani, and 11 others

Git Merge Request #311: Adding current reaction indicator (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Shalitha Suranga wants to merge 8 commits from /u/shalithasuranga/allura/ to master, 2018-12-12

Fix for [#8263]

Commit	Date
[5bed49] (patch11) by Shalitha Suranga [#8263] Add reaction indicator near the comment box	2018-12-11 07:56:57	Tree
[a001a5] by Shalitha Suranga [#8263] Move methods from app_globals to helpers	2018-12-11 07:54:40	Tree
[6598d9] by Shalitha Suranga [#8263] Fix div resizing issue	2018-11-28 04:26:50	Tree
[8cc2be] by Shalitha Suranga [#8263] Add styles for current reaction indication	2018-11-28 04:20:18	Tree
[a28524] by Shalitha Suranga [#8263] Identify current selected emoji of comment	2018-11-28 03:24:15	Tree
[e874b1] by Shalitha Suranga Merge branch 'master' of https://git-wip-us.apache.org/repos/asf/allura	2018-11-27 07:40:28	Tree
[c6a6ab] by Shalitha Suranga Merge branch 'master' of https://github.com/shalithasuranga/allura	2018-11-27 07:39:58	Tree
[4f7fba] by Shalitha Suranga , pushed by GitHub Merge pull request #1 from apache/master Taking latest changes	2018-10-05 04:33:18	Tree

Discussion

Shalitha Suranga - 2018-11-28

Notice. Since data is passed to template from a template through widget. I had to create new method in app_globals.py

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2018-11-30

Minor feedback on the code: personally I would put get_current_reaction in helpers.py (and then it'd be used as h.get_current_reaction) instead of in app_globals.py I think of globals as data and instances of objects that only exist once as shared global state. I guess emojize could be moved too, I didn't realize that during earlier work.

As for the display & functionality, I was actually thinking that the reactions div under the comment text would be where we indicate the current reaction. That way you can see your reaction without having to click on the action button. What you have done is a good first step though for sure. Also, my idea of displaying it below would imply that you can click on the emoji there to remove your reaction, which may complicate things having 2 places to remove a reaction. What do you think?

👍
1

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Shalitha Suranga - 2018-12-09

Hi.. Dave I will check about moving stuff to helpers.py. Yeah the current way requires a click to indicate whether user reacted or not. I think we have these options

keep current work + hightlight reaction in comment section too

keep current work + make reaction list in comment section interactive (As you said this will complicate our work)

How about selecting first option and doing other option later with another MR

your idea?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Dave Brondsema - 2018-12-10
  
  That sounds fine to me
  
  👍
  1
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Shalitha Suranga - 2018-12-11

Hi.. Dave

I made several commits for these changes.

moved methods from g to h

Added current reaction indicator to bottom of comment

I had to make divs in to one line since there was issue with whitespace + inline block.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2018-12-12

Status: open --> merged
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Log in to post a comment.