Meetings module adjustments #14086

bsatarnejad · 2023-11-03T08:51:05Z

Add "Add notes" option to the agenda item more menu.
Remove the words "Added by" from agenda items (all viewports).
Add "." after the "Last updated {time}" text at the top.
When a meeting is closed, the more button (...) disappears as it should.
Make the "Duration" field in the agenda item shorter and change the placeholder text from "Duration (in minutes)" to "Duration (min)"
The "Show/hide {n} more" link should go to the bottom of the participant list when it is expanded.
Mobile: The "Meeting details" section should be at the top (before agenda items).
Mobile, the meeting status part is shortened (the text is removed and the Status and "Close meeting" button are in the same line).

…ect into meetings-module-adjustments

HDinger

Hi @bsatarnejad

It is already in quite a good state. There are two major blockers for me:

Please reduce the CSS file code, as we agreed to only use if for layouting
The code for the participants is now completely doubled. Please move what is need to the component itself and add a parameter to show all participants inline. Thus you'd only have to call the component, and no second controller or additional methods.

Further, I found some things while clicking through:

There is no space between the "Sho/hide more" button and the last user

There is a space in between the list of users

It felt weird to read "Show/hide X more" on mobile, as there was nothing shown at all. Maybe we want to change that text to something like "Show/Hide X users"

HDinger · 2023-11-10T11:25:57Z

frontend/src/stimulus/controllers/dynamic/meetings-sidebar-participants.controller.ts

+
+import { Controller } from '@hotwired/stimulus';
+
+export default class MeetingsSidebarParticipantsController extends Controller {


This is literally the same code as the MeetingsSidebarDetailsController just with a different name. Please avoid that dubplication and use the same controller for the same logic.

HDinger · 2023-11-10T11:31:08Z

modules/meeting/app/components/meeting_agenda_items/item_component/show_component.html.erb

@@ -1,5 +1,5 @@
 <%=
-  grid_layout('op-meeting-agenda-item', tag: :div) do |grid|
+  grid_layout((meeting_open? ? 'op-meeting-agenda-item' : 'closed-meeting op-meeting-agenda-item'), tag: :div) do |grid|


The first parameter of the grid-layout is used for the grid-defintions, so please do not use it for plain classes. Further, closed-meeting should be a modifier of the agenda-item, e.g. op-meeting-agenda-item--closed

HDinger · 2023-11-10T11:38:45Z

modules/meeting/app/components/meeting_agenda_items/item_component/show_component.sass

+    grid-template-columns: 20px auto 1fr minmax(auto, $meeting-agenda-item--author-width)
+    grid-template-areas: "drag-handle content duration author" ". notes notes notes notes"


I see multiple issues with that:

The template-areas definition is invalid, as there are too many columns defined in the second row.

I think the whole extra definition as well as the modifier is not needed if you change the original defintion if the last column to from 40px to fit-content(40px). I admit, that I have not tested it in detail, but it would be nice to get rid of the extra stuff.

yeah it works correctly

HDinger · 2023-11-10T11:39:28Z

modules/meeting/app/components/meetings/sidebar/details_component.html.erb

+          details.with_row(mt: 2, classes: 'op-meeting-sidebar-details-participants') do
+            flex_layout do |flex|
+              render(Primer::Beta::Octicon.new(icon: 'people'))
+              flex.with_row(classes: 'd-none', data: { 'meetings-sidebar-details-target': "hiddenParticipants" }) do


Please do not set the primer classes by yourself, but use the system_arguments for that.

HDinger · 2023-11-10T11:40:35Z

modules/meeting/app/components/meetings/sidebar/details_component.html.erb

+          details.with_row(mt: 2, classes: 'op-meeting-sidebar-details-participants') do
+            flex_layout do |flex|
+              render(Primer::Beta::Octicon.new(icon: 'people'))
+              flex.with_row(classes: 'd-none', data: { 'meetings-sidebar-details-target': "hiddenParticipants" }) do


Please do not set the primer classes by yourself, but use the system_arguments for that.

Please also change that in the sidebar, where the same thing is done.

HDinger · 2023-11-10T11:42:01Z

modules/meeting/app/components/meetings/sidebar/details_component.rb

+    def render_participant(participant)
+      flex_layout(align_items: :center) do |flex|
+        flex.with_column(classes: 'ellipsis') do
+          render(Users::AvatarComponent.new(user: participant.user,
+                                            size: :medium,
+                                            classes: 'op-principal_flex'))
+        end
+        render_participant_state(participant, flex)
+      end
+    end
+
+    def render_participant_state(participant, flex)
+      if participant.attended?
+        flex.with_column(ml: 1) do
+          render(Primer::Beta::Text.new(font_size: :small, color: :subtle)) { t("description_attended").capitalize }
+        end
+      elsif participant.invited?
+        flex.with_column(ml: 1) do
+          render(Primer::Beta::Text.new(font_size: :small, color: :subtle)) { t("description_invite").capitalize }
+        end
+      end
+    end
+


This code is duplicated, so I think it is worth to move it to the component itself, and just call the component.

HDinger · 2023-11-10T11:47:34Z

modules/meeting/app/components/meetings/sidebar/details_component.sass

+@import 'helpers'
+.op-meeting-sidebar-details-participants
+  display: none
+  &-button
+    padding: 0 !important
+    color: var(--content-link-color)
+
+
+
+@media screen and (max-width: $breakpoint-sm)
+  .op-meeting-sidebar-details-participants
+    display: block !important


CSS files of components should only contain layouting CSS and things that cannot be done with the system_arguments. That means, this whole file should be not needed. You can define the display value depending on the breakpoint like this: d: : [:none, :block, :none, :none, :none]. See here for more information. The color and the padding can also be defined via the system_arguments.

HDinger · 2023-11-10T11:49:42Z

modules/meeting/app/components/meetings/sidebar/state_component.sass

+@media screen and (max-width: $breakpoint-sm)
+  .op-meeting-sidebar-state
+    flex-direction: row !important
+    justify-content: space-between
+    align-items: center
+    &-edit
+      margin-top: 0 !important


Same as before: All of this could be shown with system_arguments. I am fine with keeping the flex defintion, but please remove the margin, as well as the extra class for it.

…ect into meetings-module-adjustments

bsatarnejad added 4 commits November 3, 2023 09:11

Remove the words Added by from agenda item

539b385

remove dot after author name

406126f

Remove the words Added by from agenda item

5136159

remove dot after author name

a93c78c

bsatarnejad force-pushed the meetings-module-adjustments branch from 406126f to a93c78c Compare November 6, 2023 10:04

bsatarnejad added 5 commits November 6, 2023 12:11

Merge branch 'meetings-module-adjustments' of github.com:opf/openproj…

eba2ab5

…ect into meetings-module-adjustments

hide item actions when meeting is closed

43263d9

Remove the words Added by from agenda item

d54aefe

remove dot after author name

7ed5cf5

hide item actions when meeting is closed

679bbbc

bsatarnejad force-pushed the meetings-module-adjustments branch from 43263d9 to 679bbbc Compare November 7, 2023 07:57

bsatarnejad added 12 commits November 7, 2023 11:47

reduce width of duration field

5856348

Merge branch 'meetings-module-adjustments' of github.com:opf/openproj…

841c794

…ect into meetings-module-adjustments

add add notes button to actions of agenda item

c8d8fe2

fix focus on ckeditor when adding note

7d175ff

move show/hide button to the bottom

e076524

Remove the words Added by from agenda item

0987bd3

remove dot after author name

909ce25

hide item actions when meeting is closed

62abff8

reduce width of duration field

26fa18c

add add notes button to actions of agenda item

a08bb0d

fix focus on ckeditor when adding note

6b3692f

move show/hide button to the bottom

6ad05f1

bsatarnejad force-pushed the meetings-module-adjustments branch from e076524 to 6ad05f1 Compare November 9, 2023 08:45

bsatarnejad added 6 commits November 9, 2023 12:00

The Meeting details section should be at the top (before agenda items)

dd6fef3

shorten the meeting status part

96d2eba

Merge branch 'meetings-module-adjustments' of github.com:opf/openproj…

2f8abfc

…ect into meetings-module-adjustments

remove participants in mobile mode and show them in details part

00f1d11

Remove the words Added by from agenda item

ca149d7

remove dot after author name

02701d3

bsatarnejad added 4 commits November 9, 2023 16:15

move show/hide button to the bottom

b523780

The Meeting details section should be at the top (before agenda items)

1da4496

shorten the meeting status part

625b434

remove participants in mobile mode and show them in details part

0489328

bsatarnejad force-pushed the meetings-module-adjustments branch from 00f1d11 to 0489328 Compare November 9, 2023 15:15

HDinger requested changes Nov 10, 2023

View reviewed changes

bsatarnejad added 9 commits November 13, 2023 11:17

use participants component in details component

a68d3f9

Merge branch 'meetings-module-adjustments' of github.com:opf/openproj…

6443cfe

…ect into meetings-module-adjustments

fix merge error

a7a3454

change margin top value for hidden participants

db0dde4

remove duplicated code and unnecessary files

d4054f2

use system argument instead of custom classes

7430dfe

change class name for changing template of closed meeting

fb4731d

remove unnecessary identifier for closed meeting

51e5f81

Merge branch dev into meetings-module-adjustments

9dd8047

bsatarnejad requested a review from HDinger November 14, 2023 06:35

bsatarnejad added 2 commits November 14, 2023 08:05

Merge branch 'dev' into meetings-module-adjustments

4a5e355

fix failing feature test

beb4df1

bsatarnejad marked this pull request as ready for review November 14, 2023 07:40

bsatarnejad and others added 6 commits November 14, 2023 14:49

remove participant component from details component

af1dfad

undo changes to show participants inline

047c824

remove unnecessary methods and strings

43a0277

Merge branch 'dev' into meetings-module-adjustments

625653b

fix unit test error

56963e7

Merge branch 'meetings-module-adjustments' of github.com:opf/openproj…

a2e52e8

…ect into meetings-module-adjustments

bsatarnejad requested review from HDinger and removed request for HDinger November 15, 2023 12:59

oliverguenther approved these changes Nov 16, 2023

View reviewed changes

oliverguenther merged commit 2663f19 into dev Nov 16, 2023
11 checks passed

oliverguenther deleted the meetings-module-adjustments branch November 16, 2023 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meetings module adjustments #14086

Meetings module adjustments #14086

bsatarnejad commented Nov 3, 2023 •

edited

Loading

HDinger left a comment •

edited by bsatarnejad

Loading

HDinger Nov 10, 2023

HDinger Nov 10, 2023

HDinger Nov 10, 2023

bsatarnejad Nov 13, 2023

HDinger Nov 10, 2023

HDinger Nov 10, 2023

HDinger Nov 10, 2023

HDinger Nov 10, 2023

HDinger Nov 10, 2023


		import { Controller } from '@hotwired/stimulus';

		export default class MeetingsSidebarParticipantsController extends Controller {

		grid-template-columns: 20px auto 1fr minmax(auto, $meeting-agenda-item--author-width)
		grid-template-areas: "drag-handle content duration author" ". notes notes notes notes"

Meetings module adjustments #14086

Meetings module adjustments #14086

Conversation

bsatarnejad commented Nov 3, 2023 • edited Loading

HDinger left a comment • edited by bsatarnejad Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsatarnejad commented Nov 3, 2023 •

edited

Loading

HDinger left a comment •

edited by bsatarnejad

Loading