Standup meeting group refactor #138

toyhammered · 2023-07-03T04:42:15Z

What's the change?

Refactors Standup Meeting group Index
removes dynamic buttons from joinable items
Adds joined card component
Adds ability to join and leave standup meeting groups from Index
Styled view and components to reflect wireframes

Notes:

This PR successfully addresses the majority of the specifications outlined in the provided wireframes design. However, we made the decision to split two functionalities into separate issues for better management and clarity.

Check-in buttons in the joined_card components
Checking messages in joined_card components

These two features turned out to be more involved than initially anticipated. Separating them allows us to focus on their implementation more effectively. Also, potential updates outside of this issue might improve how we address them.

Checklist before requesting a review

Did you add appropriate automated tests?
Did you consider risks around security, performance, etc.?
Have you thought of misfiring code? e.g. too many loops, n+1, or how to handle nils?
Related to readability and consistency: Did you add comments and/or documentation? (README, Notion, etc.)
Is everything elevated to the highest level? (e.g., can logic be moved out of view into controllers, or out of controllers into models?)
Did you leave helpful inline PR comments for the reviewer(s)?
Did you include instructions for how to test in the ticket?
Did you add appropriate automated tests?

Screen.Recording.2023-07-25.at.1.02.32.PM.mov

Screen.Recording.2023-07-29.at.12.10.59.PM.mov

…-learning/PairApp into standup-meeting-group-refactor

joined_card_component.rb co-author: Roc Devenport rubykod@gmail.com

destroy action co-author: Roc Devenport rubykod@gmail.com

joined_card_component.html.erb co-authored-by: Roc Devenport rubykod@gmail.com

controller to render turbo stream on destroy co-authored-by: RocDavenport rubykod@gmail.com

JoshDevHub · 2023-07-22T03:23:03Z

@GALTdea Make sure you fix the CI failures. Looks like something isn't passing lint.

afogel · 2023-07-22T05:42:19Z

it would also be helpful if you could add screenshots/flesh out the context for the reviewer, e.g. https://fogel.dev/software/what_makes_a_good_pr/

afogel · 2023-07-22T05:41:03Z

app/components/flash_component.rb

@@ -2,6 +2,7 @@

 class FlashComponent < ViewComponent::Base
  TYPE_THEMES = {
+    success: 'success',


lol, while I generally agree that this should probably be success across the board rather than notice, I'm wondering whether this is the PR to do it or whether we can use the existing notice Flash and kick updating to success to another PR?

GALTdea · 2023-07-22T16:06:19Z

@GALTdea Make sure you fix the CI failures. Looks like something isn't passing lint.

Thanks for the review Josh! I did see one error that was related to a variable length, I'll try to fix it today!

GALTdea · 2023-07-22T16:08:25Z

it would also be helpful if you could add screenshots/flesh out the context for the reviewer, e.g. https://fogel.dev/software/what_makes_a_good_pr/

Oh that's right, I forgot 😅. Thanks for the review Ariel

…ps_controller.rb

codecov · 2023-07-22T17:53:05Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.52% 🎉

Comparison is base (908b3f0) 96.57% compared to head (11bf6c0) 97.10%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   96.57%   97.10%   +0.52%     
==========================================
  Files         119      123       +4     
  Lines        1488     1518      +30     
==========================================
+ Hits         1437     1474      +37     
+ Misses         51       44       -7

Files Changed	Coverage Δ
app/components/flash_component.rb	`100.00% <ø> (ø)`
...s/standup_meeting_group/joinable_item_component.rb	`100.00% <100.00%> (ø)`
...nts/standup_meeting_group/joined_card_component.rb	`100.00% <100.00%> (ø)`
app/models/standup_meeting_group.rb	`90.90% <100.00%> (+2.02%)`	⬆️
...ndup_meeting_group/joinable_item_component_spec.rb	`100.00% <100.00%> (ø)`
...tandup_meeting_group/joined_card_component_spec.rb	`100.00% <100.00%> (ø)`

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

afogel

Good first attempt! That said, I think there's a bit more work beyond getting the core functionality to work.

app/components/standup_meeting_group/joined_card_component.html.erb

_standup_meeting_group_list_item.erb

display 3 cards per row.

JoshDevHub

Some comments and nits about the markup and CSS but this is looking like good progress.

Most urgent things:

A bit worried about the Card container wrapping on mobile. It doesn't at all for me, but judging from your videos, it does for you. I'm definitely interested in getting to the bottom of that. What browser are you using?
The dynamic buttons on the Cards should definitely be added before merging this imo.

app/components/standup_meeting_group/joinable_item_component.html.erb

app/components/standup_meeting_group/joined_card_component.html.erb

JoshDevHub · 2023-07-26T00:58:35Z

app/components/standup_meeting_group/joined_card_component.html.erb

+    <div class="w-1/2">
+      <div class="card-actions flex space-x-2 justify-end">
+        <button class="btn btn-sm">Check In</button>
+        <button class="btn btn-sm">Dismiss</button>


There are still plans to add the dynamic behavior here, right?

you mean checkin and dismiss? (btw changed the button name to skip). Seems like this could be a different issue, but I'll ask @toyhammered

app/controllers/standup_meeting_groups/joins_controller.rb

JoshDevHub · 2023-07-26T01:06:34Z

app/controllers/standup_meeting_groups/joins_controller.rb


    respond_to do |format|
      format.turbo_stream do
-        render turbo_stream: turbo_stream.update(helpers.dom_id(@standup_meeting_group, :join_or_leave), component)
+        flash.now[:notice] = "You have joined #{@standup_meeting_group.name}!"
+        render 'standup_meeting_groups/joins/create'


Pretty sure this render call is unnecessary.

JoshDevHub · 2023-07-26T01:08:20Z

app/components/standup_meeting_group/joined_card_component.html.erb

+        <button class="btn btn-sm">Check In</button>
+        <button class="btn btn-sm">Dismiss</button>
+        <br>
+        <%= link_to 'Leave', standup_meeting_group_join_path(@standup_meeting_group, @standup_meeting_group_user), data: { turbo_method: :delete }, class: 'btn btn-link align-bottom' %>


It looks like they've pulled leaving off the card in the wireframes. Maybe worth asking Daniel or Ariel about this and if they still want it here.

hmm, yes I see. I'll post a question in discord

app/views/standup_meeting_groups/_standup_meeting_group_list_item.erb

app/views/standup_meeting_groups/index.html.erb

…action

github.com:agency-of-learning/PairApp into standup-meeting-group-refactor

JoshDevHub

Just a couple of small things to fix and I think it's good to merge. Looks awesome and this code is definitely quite a bit simpler than what was here previously. Awesome work

app/views/standup_meeting_groups/index.html.erb

app/views/standup_meeting_groups/joins/create.turbo_stream.erb

grid to flexbox

to render JoinableItemComponent

JoshDevHub

Awesome work!

Out of date

toyhammered and others added 9 commits July 2, 2023 20:47

wip

6ac2d4b

wip

98c2a28

Merge branch 'standup-meeting-group-refactor' of github.com:agency-of…

9b3ddb6

…-learning/PairApp into standup-meeting-group-refactor

Define @standup_meeting_group_user variable in

cbecd31

joined_card_component.rb co-author: Roc Devenport rubykod@gmail.com

Add standup_meeting_groups::joins_controller.rb

4d047a2

destroy action co-author: Roc Devenport rubykod@gmail.com

Defines @standup_meeting_group_user variable in

c4ea9da

joined_card_component.html.erb co-authored-by: Roc Devenport rubykod@gmail.com

.

69c8a30

Merge branch 'main' into standup-meeting-group-refactor

aa83d79

Adds destroy.turbo_stream.erb file and updates

ca55598

controller to render turbo stream on destroy co-authored-by: RocDavenport rubykod@gmail.com

GALTdea requested review from JoshDevHub and afogel July 22, 2023 02:34

GALTdea marked this pull request as ready for review July 22, 2023 02:35

afogel reviewed Jul 22, 2023

View reviewed changes

GALTdea added 4 commits July 22, 2023 09:36

fixes linting error

bea3dc4

breaks @joinable_standup_meeting_groups line the standup_meeting_grou…

14a0be0

…ps_controller.rb

.

5580a6d

.

54f88c6

toyhammered assigned toyhammered, GALTdea and RocDavenport and unassigned toyhammered Jul 22, 2023

afogel previously requested changes Jul 22, 2023

View reviewed changes

app/components/standup_meeting_group/joined_card_component.html.erb Outdated Show resolved Hide resolved

GALTdea added 3 commits July 22, 2023 15:01

wip: adding joinable_item_component specs

f5ec015

removes join_button_component

fc8936b

removes leave_button_component

fcb7f14

GALTdea added 5 commits July 22, 2023 20:35

removed base_botton_component

7c427e2

Remove uneeded code from

bab7ab7

_standup_meeting_group_list_item.erb

adds styling changes to joined_card_component.html.erb

da17fe4

Refactores standup meeting group index to only

bdbf5a4

display 3 cards per row.

clean code

a44fe5d

JoshDevHub requested changes Jul 26, 2023

View reviewed changes

GALTdea added 6 commits July 26, 2023 17:50

Address pr comments

d12b3bb

centered joined_card_components on small screen

65b5aa2

deleted commented out code

d1fffff

wip: adding checkin_status feature to joined_cards

71fbbe9

reverting to split up issues

9238c12

adds icons and flash text for checkins messages

017e16b

GALTdea force-pushed the standup-meeting-group-refactor branch from 5bfb79e to 017e16b Compare July 29, 2023 18:31

GALTdea added 6 commits July 29, 2023 11:36

cleans code for pr

78f53e3

Adds test for joinableItemComponent

498b5f4

adds joined card component test

5de4a3a

fixes rubocop errors

5f27027

removes turbo from leave button and adds a redirect to joins destroy …

77367a5

…action

Merge branch 'main' of

9e588d2

github.com:agency-of-learning/PairApp into standup-meeting-group-refactor

JoshDevHub requested changes Jul 31, 2023

View reviewed changes

app/views/standup_meeting_groups/index.html.erb Outdated Show resolved Hide resolved

app/views/standup_meeting_groups/joins/create.turbo_stream.erb Show resolved Hide resolved

GALTdea added 3 commits July 31, 2023 16:03

Changes joined card components container from

a6b3a6d

grid to flexbox

refactor standup_meeting_groups turbo_stream

d575422

to render JoinableItemComponent

remove _standup_meeting_group_list item partial

11bf6c0

GALTdea requested a review from JoshDevHub August 1, 2023 00:52

JoshDevHub approved these changes Aug 1, 2023

View reviewed changes

JoshDevHub merged commit 8b767a1 into main Aug 1, 2023
3 checks passed

JoshDevHub deleted the standup-meeting-group-refactor branch August 1, 2023 04:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standup meeting group refactor #138

Standup meeting group refactor #138

toyhammered commented Jul 3, 2023 •

edited by GALTdea

Loading

JoshDevHub commented Jul 22, 2023

afogel commented Jul 22, 2023

afogel Jul 22, 2023

GALTdea commented Jul 22, 2023

GALTdea commented Jul 22, 2023

codecov bot commented Jul 22, 2023 •

edited

Loading

afogel left a comment

JoshDevHub left a comment

JoshDevHub Jul 26, 2023

GALTdea Jul 27, 2023

JoshDevHub Jul 26, 2023

JoshDevHub Jul 26, 2023

GALTdea Jul 26, 2023

JoshDevHub left a comment

JoshDevHub left a comment

Standup meeting group refactor #138

Standup meeting group refactor #138

Conversation

toyhammered commented Jul 3, 2023 • edited by GALTdea Loading

What's the change?

Notes:

Checklist before requesting a review

JoshDevHub commented Jul 22, 2023

afogel commented Jul 22, 2023

afogel Jul 22, 2023

Choose a reason for hiding this comment

GALTdea commented Jul 22, 2023

GALTdea commented Jul 22, 2023

codecov bot commented Jul 22, 2023 • edited Loading

Codecov Report

afogel left a comment

Choose a reason for hiding this comment

JoshDevHub left a comment

Choose a reason for hiding this comment

JoshDevHub Jul 26, 2023

Choose a reason for hiding this comment

GALTdea Jul 27, 2023

Choose a reason for hiding this comment

JoshDevHub Jul 26, 2023

Choose a reason for hiding this comment

JoshDevHub Jul 26, 2023

Choose a reason for hiding this comment

GALTdea Jul 26, 2023

Choose a reason for hiding this comment

JoshDevHub left a comment

Choose a reason for hiding this comment

JoshDevHub left a comment

Choose a reason for hiding this comment

toyhammered commented Jul 3, 2023 •

edited by GALTdea

Loading

codecov bot commented Jul 22, 2023 •

edited

Loading