Manage listing categories in dashboard #10506

Rafi993 · 2020-10-01T03:33:20Z

What type of PR is this?

Description

It adds ability to add/update/remove listing categories in admin panel

Related Tickets & Documents

closes #10481

QA Instructions, Screenshots, Recordings

https://vimeo.com/463735841/dfe6705dab

Added tests?

yes
no, because they aren't needed
no, because I need help

Added to documentation?

docs.forem.com
readme
no documentation needed

Rafi993 · 2020-10-01T08:19:22Z

app/lib/constants/role.rb

@@ -18,6 +18,7 @@ module Role
                     "Resource Admin: Config",
                     "Resource Admin: Broadcast",
                     "Resource Admin: HtmlVariant",
-                     "Resource Admin: DisplayAd"].freeze
+                     "Resource Admin: DisplayAd",
+                     "Resource Admin: ListingCategory"].freeze


I am not sure if it should be called Listing or ListingCategory

as it's a category, I think ListingCategory is correct

Rafi993 · 2020-10-01T08:58:52Z

I think some unrelated test is failing

rhymes · 2020-10-01T09:36:29Z

@Rafi993 restarted, cross fingers :D

fdocr

Thank you for the PR @Rafi993, great job in following the sample PR from the issue!

Functionality-wise the PR is working great locally.

I have a small change I think it would be good to include and another non-blocking suggestion. I checked the sample PR from the issue and just now noticed this one slipped in there, so not your fault at all!

With this tiny change in place I'd say this looks good.

fdocr · 2020-10-01T15:59:08Z

app/views/admin/listings/index.html.erb

  <h2 class="fs-2xl s:fs-3xl mb-4">Listings</h2>
+  <a href="/admin/listings/categories"
+  aria-label="Listing Categories"
+  class="ml-auto">


Non-blocking suggestion:

Suggested change

class="ml-auto">

class="ml-auto crayons-btn">

fdocr · 2020-10-01T16:06:29Z

app/controllers/admin/listing_categories_controller.rb

+        flash[:success] = "Listing Category has been deleted!"
+        redirect_to admin_listing_categories_path
+      else
+        flash[:danger] = "Something went wrong with deleting the Listing Category."


It's best to do the same as the update action so the error message will be descriptive

Suggested change

flash[:danger] = "Something went wrong with deleting the Listing Category."

flash[:danger] = @listing_category.errors.full_messages.to_sentence

Rafi993 · 2020-10-01T16:30:04Z

Thank you for the PR @Rafi993, great job in following the sample PR from the issue!

Functionality-wise the PR is working great locally.

I have a small change I think it would be good to include and another non-blocking suggestion. I checked the sample PR from the issue and just now noticed this one slipped in there, so not your fault at all!

With this tiny change in place I'd say this looks good.

Thank you @fdoxyz I'll make those changes.

fdocr

Awesome thanks again @Rafi993!

The codeclimate errors can be ignored since they're complaining about similar code in different places. So far we've considered it's best to maintain a similar approach in different admin controllers and maybe refactor later on in a way that would make this go away (or get rid of the codeclimate rule for this particular scenario).

juliannatetreault

Thank you so much for adding Listing Categories management to /admin, @Rafi993! I tested everything out locally and it all worked really well. One thing that I did notice though, is that the placeholder text in the search bar, found under /admin/listings/categories, on a 15-inch screen on Google Chrome is slightly cut off (shown in the picture below). That aside, everything looked and worked great! 🎉

mstruve

Tested, works great!

Rafi993 · 2020-10-02T02:14:32Z

Thank you so much for adding Listing Categories management to /admin, @Rafi993! I tested everything out locally and it all worked really well. One thing that I did notice though, is that the placeholder text in the search bar, found under /admin/listings/categories, on a 15-inch screen on Google Chrome is slightly cut off (shown in the picture below). That aside, everything looked and worked great!

Thank you @juliannatetreault I did try to emulate 15 inch mac with and without retina display in chrome dev tools with following values

size 2880 x 1800
I tried device pixel ration 1, 2 and 3 (default value for my screen is 2)
My screen size is 14 inches hdpi screen

I also tried couple of other screen resolutions too but was unable to reproduce this issue.

fdocr · 2020-10-02T02:54:48Z

I do get the same effect on 13 inch chrome, but also ran into this over with DisplayAds

I couldn't find a way to make the input grow horizontally there (I tried a few things from https://storybook.forem.com/).

A suggestion: Shortening the placeholder to "Search by name" would make it fit (like in DisplayAds) and it wouldn't be cut out.
Maybe nice to have: A fix for the element to grow horizontally (could also be used to enhance other dashboards like this one)

Rafi993 · 2020-10-02T04:53:31Z

I do get the same effect on 13 inch chrome, but also ran into this over with DisplayAds

I couldn't find a way to make the input grow horizontally there (I tried a few things from https://storybook.forem.com/).

A suggestion: Shortening the placeholder to "Search by name" would make it fit (like in DisplayAds) and it wouldn't be cut out.
Maybe nice to have: A fix for the element to grow horizontally (could also be used to enhance other dashboards like this one)

Thank you @fdoxyz I have changed the text to "Search by name" now.

Rafi993 · 2020-10-02T05:02:20Z

Thank you @fdoxyz should I revert the "search by name" placeholder change?

fdocr · 2020-10-02T05:10:32Z

So sorry @Rafi993 that's my bad! I was checking in to see if CI had failed on mobile and closed by accident 🙈

I'd love to give the opportunity for other different timezones to weigh in, we may get a suggestion for manipulating the input field a little better. It still has my approval 🙌

ludwiczakpawel · 2020-10-02T08:53:03Z

There's not an easy way (right now!) to manipulate easily the width of the input.. Changing the label to be shorter is a way to go IMO: "Search by name" and call it a day :)

We can of course do some hacky things but I don't think it's worth it :)

juliannatetreault

Thank you for making the change to the placeholder text, @Rafi993! I think this PR is good-to-go now. Thanks again for doing this! 🎉

Rafi993 · 2020-10-02T14:09:29Z

Thank you for making the change to the placeholder text, @Rafi993! I think this PR is good-to-go now. Thanks again for doing this!

Thank you @juliannatetreault

Rafi993 added 2 commits September 30, 2020 22:47

Displaying all listing

c720f66

Edit listing form

8032f39

pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Oct 1, 2020

Rafi993 added 6 commits October 1, 2020 10:26

Editing listing

4809546

Searching listing categories

97ac885

Creating new listing category

b3caaea

Deleting listing category

38c835d

Fixing error message and roles

891317e

Tests

8a8daf2

Rafi993 commented Oct 1, 2020

View reviewed changes

Fixing aria label

64f6cdc

Rafi993 marked this pull request as ready for review October 1, 2020 08:33

Rafi993 requested a review from a team October 1, 2020 08:33

Rafi993 requested a review from a team as a code owner October 1, 2020 08:33

Rafi993 requested review from juliannatetreault and removed request for a team October 1, 2020 08:33

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Oct 1, 2020

rhymes requested review from fdocr and removed request for a team October 1, 2020 08:52

rhymes changed the title ~~Rafi993/manage listing in dashboard~~ Manage listings in dashboard Oct 1, 2020

rhymes changed the title ~~Manage listings in dashboard~~ Manage listing categories in dashboard Oct 1, 2020

fdocr suggested changes Oct 1, 2020

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 1, 2020

Rafi993 added 2 commits October 1, 2020 22:18

Fixing link class

1bd8757

Fixing error message of destroy

9b4c7b6

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 1, 2020

fdocr approved these changes Oct 1, 2020

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 1, 2020

juliannatetreault reviewed Oct 1, 2020

View reviewed changes

mstruve approved these changes Oct 1, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 1, 2020

Changing placeholder text

3120f40

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 2, 2020

fdocr closed this Oct 2, 2020

fdocr reopened this Oct 2, 2020

Merge branch 'master' into Rafi993/manage-listing-in-dashboard

b4ca998

juliannatetreault approved these changes Oct 2, 2020

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 2, 2020

Zhao-Andy merged commit 4a9f271 into forem:master Oct 2, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 2, 2020

Manage listing categories in dashboard #10506

Manage listing categories in dashboard #10506

Rafi993 commented Oct 1, 2020 •

edited

Rafi993 Oct 1, 2020

rhymes Oct 1, 2020

Rafi993 commented Oct 1, 2020

rhymes commented Oct 1, 2020

fdocr left a comment

fdocr Oct 1, 2020

fdocr Oct 1, 2020

Rafi993 commented Oct 1, 2020

fdocr left a comment

juliannatetreault left a comment

mstruve left a comment

Rafi993 commented Oct 2, 2020

fdocr commented Oct 2, 2020

Rafi993 commented Oct 2, 2020

Rafi993 commented Oct 2, 2020

fdocr commented Oct 2, 2020

ludwiczakpawel commented Oct 2, 2020

juliannatetreault left a comment

Rafi993 commented Oct 2, 2020

	flash[:danger] = "Something went wrong with deleting the Listing Category."
	flash[:danger] = @listing_category.errors.full_messages.to_sentence

Manage listing categories in dashboard #10506

Manage listing categories in dashboard #10506

Conversation

Rafi993 commented Oct 1, 2020 • edited

What type of PR is this?

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added tests?

Added to documentation?

Rafi993 Oct 1, 2020

Choose a reason for hiding this comment

rhymes Oct 1, 2020

Choose a reason for hiding this comment

Rafi993 commented Oct 1, 2020

rhymes commented Oct 1, 2020

fdocr left a comment

Choose a reason for hiding this comment

fdocr Oct 1, 2020

Choose a reason for hiding this comment

fdocr Oct 1, 2020

Choose a reason for hiding this comment

Rafi993 commented Oct 1, 2020

fdocr left a comment

Choose a reason for hiding this comment

juliannatetreault left a comment

Choose a reason for hiding this comment

mstruve left a comment

Choose a reason for hiding this comment

Rafi993 commented Oct 2, 2020

fdocr commented Oct 2, 2020

Rafi993 commented Oct 2, 2020

Rafi993 commented Oct 2, 2020

fdocr commented Oct 2, 2020

ludwiczakpawel commented Oct 2, 2020

juliannatetreault left a comment

Choose a reason for hiding this comment

Rafi993 commented Oct 2, 2020

Rafi993 commented Oct 1, 2020 •

edited