Feature/chapters index page #1069

yihyang · 2019-09-30T15:21:38Z

Issue: #738

Screenshot:

matyikriszta · 2019-10-03T04:09:45Z

@yihyang thanks for the PR, please give me a few days to review it, I'll try to get to it this week

notapatch

re "trust me I'm a programmer". Don't need to trust you as your pull request say you're a programmer. The code was immaculate. 👍

I'm a contributor - I can't approve your work and I don't know anymore about the issue than I've read. These are just my opinions. If you don't agree with them feel free to ignore. You've already met the maintainer @matyikriszta.

I would squash the commits - I think it just makes it more difficult seeing the changes - I agree with all of the code climate changes.
The grammar change throughout the test file (see other comment).
The UX changes are listed below
1. Margin between links
2. Active links on the index to the item
3. Toggling active/not-active with checkbox

1 margin between links

The links need a bit more margin between the links (see below).

2. Active links on the index to the item

I expect that indexes link through to the actual items - am I missing the link below?

3. Toggling active/not-active with checkbox

Isn't "Active?" and actions normally combined into a checkbox? (see below)

👍

notapatch · 2019-11-15T00:25:29Z

spec/features/admin/chapters_spec.rb

+      assert_chapters_exist_on_page(inactive_chapters)
+    end
+
+    scenario 'an admin can filter and view only active chapters' do


"filter and view" => "filter to view"

Personally, I would have written "an admin can view only active chapters" the filter is an implementation detail which is already implied - I don't have strong view on this.

Hi @notapatch thanks for the suggestions, I have updated it in 91c732d

yihyang · 2020-05-17T15:33:14Z

Hi @matyikriszta @notapatch , sorry for the late reply. I hope that everyone is safe during this times. I have updated the PR as per suggestion, please have a look, thanks!

yihyang · 2020-05-17T15:41:31Z

spec/support/wait_for_ajax.rb

@@ -0,0 +1,15 @@
+module WaitForAjax


copied this from https://thoughtbot.com/blog/automatically-wait-for-ajax-with-capybara

notapatch · 2020-05-17T16:36:14Z

Hi @yihyang

@despo, the project owner, is active but I don't know the project's priorities as codebar has had to pivot to online workshops which has led to a lot of code changes. I hope she has the time.🤞

despo

@yihyang thanks for the PR. I left some feedback.

despo · 2020-05-17T17:23:03Z

app/controllers/admin/chapters_controller.rb

+    @chapter.update(update_active_params)
+
+    head :ok
+  end


Can we stick to standard rails CRUD operations and reuse update to achieve this instead. The active param can just be part of chapter_params and the existing update method could be used to achieve the same thing.

despo · 2020-05-17T17:27:09Z

app/views/admin/chapters/index.html.haml

+      .large-1.columns
+        = check_box_tag :chapter_active, 'true', chapter.active?, class: 'chapter_active', data: { 'chapter-id': chapter.id, }
+
+- content_for :page_footer do


I don't mind having javascript available as well, however it should be optional and not the default. Also, a confirmation button to verify disabling chapters before any actions take place.

despo · 2020-05-17T17:27:55Z

app/views/admin/chapters/index.html.haml

@@ -0,0 +1,45 @@
+%section#banner
+  .row


a .row in foundation should contain column elements, so here there should be a large-12.columns to achieve correct alignment.

despo · 2020-05-17T17:28:39Z

app/views/admin/chapters/index.html.haml

+    &nbsp;
+    = link_to t('.view_inactive_chapters'), admin_chapters_path(active: false)
+    .row
+      %hr


similar to the comment above, as a row is made up of column elements, hr should be within a .large-12.columns

despo · 2020-05-17T17:29:27Z

config/routes.rb

    resources :member_notes, only: [:create]

    resources :chapters, only: %i[index new create show edit update] do
+      member do


this can be removed as per comments added above

despo · 2020-05-17T17:29:56Z

spec/features/admin/chapters_spec.rb

+
+    background { login_as_admin(member) }
+
+    scenario "an admin can view all chapters and it's information" do


could you please update to ...all chapters and their information

github-actions · 2020-12-03T22:07:15Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2021-01-03T00:14:10Z

Closing as stale.

yihyang mentioned this pull request Sep 30, 2019

Add admin chapters view that lists all active and inactive chapters with the ability to manage the status of the chapters (active/inactive) #738

Closed

7 tasks

notapatch mentioned this pull request Oct 4, 2019

[WIP] chapter with basic layer of testing #1079

Closed

notapatch reviewed Nov 15, 2019

View reviewed changes

yihyang added 4 commits May 17, 2020 22:13

implemented admin/chapters#index features

8f1db79

fixed title error

d06f63e

updated formatting based on codeclimate suggestions

269a950

changed labels to translations

f2d1698

yihyang force-pushed the feature/chapters_index_page branch from 0c3dec6 to f2d1698 Compare May 17, 2020 14:13

yihyang added 3 commits May 17, 2020 22:16

fixed naming conventioned based on rubocop's comment

228785b

updated admin chapters index page toggle

26edcf9

updated grammatical mistakes

91c732d

yihyang commented May 17, 2020

View reviewed changes

despo requested changes May 17, 2020

View reviewed changes

updated format, changed page to show confirmation on active change

f907ab9

github-actions bot added the Stale label Dec 3, 2020

github-actions bot closed this Jan 3, 2021


		background { login_as_admin(member) }

		scenario "an admin can view all chapters and it's information" do

Uh oh!

Feature/chapters index page #1069

Feature/chapters index page #1069

Uh oh!

Conversation

yihyang commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matyikriszta commented Oct 3, 2019

Uh oh!

notapatch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

1 margin between links

2. Active links on the index to the item

3. Toggling active/not-active with checkbox

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihyang commented May 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notapatch commented May 17, 2020

Uh oh!

despo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

despo May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 3, 2020

Uh oh!

github-actions bot commented Jan 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yihyang commented Sep 30, 2019 •

edited

Loading

notapatch left a comment •

edited

Loading

despo May 17, 2020 •

edited

Loading