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

spam2 stable issues, UI update and modals added #8034

Merged
merged 18 commits into from
Jun 23, 2020

Conversation

keshavsethi
Copy link
Member

@keshavsethi keshavsethi commented Jun 14, 2020

There were few bugs in the UI of the spam management dashboard.

  • Wrong ids in /comments and /revisions
  • created modal for comment and revisions
  • default order nodes wrt time
  • UI is updated as there were few issues related to banner in stable. Apart from that this new UI requires very less external css and it is based on bootstrap mostly. it has everything which was in old UI. This is fully responsive and better than the previous UI. I have attached some screenshots and gifs.
    Screenshot from 2020-06-20 18-14-00
    Screenshot from 2020-06-20 18-10-04
    Screenshot from 2020-06-20 18-09-47

There are a few issues that I am facing currently:

  1. /spam2 and /spam2/revision are giving error in stable while wiki and comment are fine, I think it might be due to invalid ID or something. I have changed it but not sure if it is the only issue.
    I have added (&.) instead of (.) it will discard all values where node.author is nill. Please review this.
    <a href="/profile/<%= node.author&.name %>"><%= node.author&.name %> <%= node.author&.new_contributor%></a>
    Please review @jywarren @SidharthBansal @cesswairimu @pydevsg @ananyaarun @VladimirMikulic @Uzay-G @emilyashley
    Thanks!!

@codecov
Copy link

codecov bot commented Jun 14, 2020

Codecov Report

Merging #8034 into master will decrease coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8034      +/-   ##
==========================================
- Coverage   82.62%   82.41%   -0.21%     
==========================================
  Files          99       99              
  Lines        5737     5737              
==========================================
- Hits         4740     4728      -12     
- Misses        997     1009      +12     
Impacted Files Coverage Δ
app/controllers/spam2_controller.rb 100.00% <100.00%> (ø)
app/services/execute_search.rb 88.88% <0.00%> (-5.56%) ⬇️
app/api/srch/search.rb 66.24% <0.00%> (-3.83%) ⬇️
app/controllers/admin_controller.rb 79.74% <0.00%> (-2.11%) ⬇️

@VladimirMikulic
Copy link
Contributor

Fix CI :)

@jywarren jywarren added this to the Spam! milestone Jun 16, 2020
@jywarren
Copy link
Member

This is a follow-up to #7961 -- right? Would you mind linking together the issues in a central place so we can see them all in relation? Thank you!

@sentry-io
Copy link

sentry-io bot commented Jun 16, 2020

Sentry issue: PLOTS2-RH

@jywarren
Copy link
Member

Hi @keshavsethi -- here, can you see this? https://sentry.io/share/issue/0d9664f6f2db4194978b7671837928ee/

Also were you able to log into Sentry and do searches and link like I've done in the previous comment? Thanks!

@jywarren
Copy link
Member

Hmm, regarding anniversary banner, is there a way to not use absolute positioning but to have it push down below if the banner exists? Otherwise, yes, we could exempt it. Thank you!!!

@jywarren
Copy link
Member

and finally, sorry, would you be able to add a little more descriptive title here? it really helps when trying to link issues together with # autocompletion. Thank you!!!!!

@jywarren jywarren mentioned this pull request Jun 16, 2020
@keshavsethi
Copy link
Member Author

This is a follow-up to #7961 -- right? Would you mind linking together the issues in a central place so we can see them all in relation? Thank you!

#7961(closed)was the first draft then I /spam2 was changed a lot. This PR is just a few updates to that including errors in the stable. Thanks!

@keshavsethi
Copy link
Member Author

Hi @keshavsethi -- here, can you see this? https://sentry.io/share/issue/0d9664f6f2db4194978b7671837928ee/

Also were you able to log into Sentry and do searches and link like I've done in the previous comment? Thanks!

@jywarren Yes, I saw error before in sentry. It was due to data: { confirm: 'Are you sure you want to delete "'+node.path+'"?' } I have changed node.path here in this PR to #{node.path}. I hope it should work now. Thanks!

@keshavsethi
Copy link
Member Author

keshavsethi commented Jun 16, 2020

Hmm, regarding anniversary banner, is there a way to not use absolute positioning but to have it push down below if the banner exists? Otherwise, yes, we could exempt it. Thank you!!!

@jywarren I have changed UI a bit, This will work fine in stable.

Thanks!

@keshavsethi
Copy link
Member Author

@jywarren @SidharthBansal @cesswairimu @pydevsg @ananyaarun @VladimirMikulic @Uzay-G @emilyashley I have changed UI a bit. Please review.
Thanks!!

@keshavsethi keshavsethi changed the title spam2 stable issues and modals added spam2 stable issues, UI update and modals added Jun 18, 2020
Copy link
Member

@pydevsg pydevsg left a comment

Choose a reason for hiding this comment

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

Good work man @keshavsethi 🎉

@@ -36,10 +36,6 @@
cursor: pointer;
}

.alert {
Copy link
Member

@pydevsg pydevsg Jun 18, 2020

Choose a reason for hiding this comment

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

hey @keshavsethi , inplace of alert what else have you added .
Also can you tell me if you have added something in CSS when a model pop ups, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alert is there, it is just a z-index for that which I have added when there was another top nav for bulk moderation(previous UI). Now I have moved that nav below the card and there is no need to specify z-index for alert. I have used a modal bootstrap class here.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@keshavsethi
Copy link
Member Author

I have added a specific column search in moderation nav.it will help moderators to filter node to a greater extent. Refer to following screenshot
Screenshot from 2020-06-18 19-13-05

@keshavsethi keshavsethi requested a review from pydevsg June 20, 2020 12:34
@jywarren
Copy link
Member

85029356-d6cb9400-b199-11ea-94e2-9e4fa1b93ee5

Hi, this is super awesome. I have a few suggestions before we merge -- some easy, some maybe for a follow-up?

  1. (A) - Could we have a “?” button too leading to /wiki/moderation#Dashboard where we add some documentation of each button?
  2. (B and C) - could we group these together better? I don't know if Bootstrap has a style for this, but adding "Filter by author" instead of just "Author" could help clarify, and grouping them together could help people understand that the input text field is the term that is being used for the filtering. Also, if it has a placeholder text (via Bootstrap styles), it could say "Filter text" or something?
  3. (D) perhaps after it says "filter" we should have a (clear) link that allows us to clear filters from that spot.

Thanks, what do you think? 🎉

@ebarry
Copy link
Member

ebarry commented Jun 23, 2020

Thanks @keshavsethi, this dashboard is looking good!
👍 to @jywarren's 3 points.
I"m also wondering if there could be text "Select content below to take any of the following actions" above the buttons, and if the buttons could be shown in paler colors until there is content selected?

@keshavsethi
Copy link
Member Author

85029356-d6cb9400-b199-11ea-94e2-9e4fa1b93ee5

Hi, this is super awesome. I have a few suggestions before we merge -- some easy, some may be for a follow-up?

  1. (A) - Could we have a “?” button too leading to /wiki/moderation#Dashboard where we add some documentation of each button?
  2. (B and C) - could we group these together better? I don't know if Bootstrap has a style for this, but adding "Filter by author" instead of just "Author" could help clarify, and grouping them together could help people understand that the input text field is the term that is being used for the filtering. Also, if it has a placeholder text (via Bootstrap styles), it could say "Filter text" or something?
  3. (D) perhaps after it says "filter" we should have a (clear) link that allows us to clear filters from that spot.

Thanks, what do you think?

Hey @jywarren, I have added an info button, I will link it with wiki page once it is made. I have made B and C as a part of a single button group and added a tooltip for it for reference. There is a reset button in the header of the table which becomes red if we apply any filter and black if no filter is applied I have also disabled bulk moderation buttons if nothing is selected. and I have updated UI a bit. please refer to the following Screenshot.
Screenshot from 2020-06-24 00-56-02
@jywarren Please review these changes, Thanks!!

@keshavsethi
Copy link
Member Author

Thanks @keshavsethi, this dashboard is looking good!
to @jywarren's 3 points.
I"m also wondering if there could be text "Select content below to take any of the following actions" above the buttons, and if the buttons could be shown in paler colors until there is content selected?

@ebarry, I have disabled the buttons if nothing is selected, Thanks!!
Screenshot from 2020-06-24 00-56-02

@jywarren
Copy link
Member

Oh WOW. @keshavsethi this is very fast work! I'll merge it now!

@jywarren jywarren merged commit 9559d6e into publiclab:master Jun 23, 2020
@jywarren
Copy link
Member

It'll be testable at https://stable.publiclab.org/spam2 soon. And if that works, i may try to push it into production today! We'll keep an eye out!!!

@jywarren
Copy link
Member

Congrats!! 🎉

@keshavsethi
Copy link
Member Author

It'll be testable at https://stable.publiclab.org/spam2 soon. And if that works, i may try to push it into production today! We'll keep an eye out!!!

Sure, Thanks!! 😄

@cesswairimu
Copy link
Collaborator

🎉 🎉

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.

None yet

6 participants