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

DEV: migrate post event to glimmer #615

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jjaffeux
Copy link
Contributor

@jjaffeux jjaffeux commented Oct 13, 2024

This commit is making two major changes:

  • Move all the post event widgets code to glimmer
  • Implement tracked models to ensure reactivity, many paths didn't have real reactivity atm and were mostly working by luck or the fact that widgets re-render a lot

@jjaffeux jjaffeux force-pushed the migrate-post-event-to-glimmer branch from 0299275 to d1e7930 Compare October 15, 2024 10:47
@jjaffeux jjaffeux changed the title wip: migrate post event to glimmer DEV: migrate post event to glimmer Oct 17, 2024
@jjaffeux jjaffeux marked this pull request as ready for review October 17, 2024 07:51
@@ -11,7 +11,7 @@ def index
event_invitees = event.invitees
event_invitees = event_invitees.with_status(params[:type].to_sym) if params[:type]

possible_invitees = []
suggested_users = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is recent code sam added, I just changed the name as we actually return users and not invitees

not_going: not_going,
invited: going + interested + not_going + unanswered,
}
EventStatsSerializer.new(object, root: false).as_json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now use this serializer at two places so I extracted it

@@ -0,0 +1,36 @@
# frozen_string_literal: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extracted serialize; no new code

@@ -12,21 +12,21 @@ def invitees

def meta
{
possible_invitees:
suggested_users:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

still name change of recent sam change

@@ -15,5 +15,14 @@ def include_id?
def user
BasicUserSerializer.new(object.user, embed: :objects, root: false)
end

def meta
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im returning these info in meta so I can properly update the model without any other remote calls when changing attendance

@@ -0,0 +1,29 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1:1 migration from widget

@@ -0,0 +1,84 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is a little bit tricky as most of it was computed in the initializer before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two cases:

  • local dates is enabled
  • local dates is disabled

@@ -0,0 +1,198 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1:1 migration, most notable change is that message bus is not setup in this component instead of the initializer

return this.currentUser && this.args.event.can_act_on_discourse_post_event;
}

get containerHeight() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part was also done in the initializer before

@@ -0,0 +1,65 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1:1 migration from widget

@@ -0,0 +1,70 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1/1 migration from widget

@@ -0,0 +1,367 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was using the widget dropdown and now is using DMenu

@@ -0,0 +1,110 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1:1 migration from widgets with PluginOutlets to support customizations we have

@@ -0,0 +1,25 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1:1 migration from widget

@@ -2,15 +2,15 @@
@title={{i18n
(concat
"discourse_calendar.discourse_post_event.builder_modal."
(if @model.event.isNew "create_event_title" "update_event_title")
(if @model.event.id "update_event_title" "create_event_title")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed isNew, technically it's persisted once we have an id

)
}}
class="post-event-builder-modal"
@closeModal={{@closeModal}}
@flash={{this.flash}}
>
<:body>
<ConditionalLoadingSection @isLoading={{@model.event.isSaving}}>
<ConditionalLoadingSection @isLoading={{this.isSaving}}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved isSaving out of the model

@action={{this.createEvent}}
/>
{{else}}
{{#if @model.event.id}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just inverted the logic here

@@ -12,29 +12,29 @@ import concatClass from "discourse/helpers/concat-class";
import i18n from "discourse-common/helpers/i18n";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a new "Invitees" model which can also old the suggested users. Not sure yet about this design but this is ok for now

@@ -0,0 +1,30 @@
import Component from "@glimmer/component";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is replacing the renderInvitee helper. We are actually rendering a User here.

@@ -75,12 +75,10 @@ export default Component.extend({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly casing changes

starts_at: moment(),
timezone: moment.tz.guess(),
});

modal.show(PostEventBuilder, {
model: {
event: eventModel,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these functions are mostly not needed anymore

status: "public",
custom_fields: EmberObject.create({}),
Copy link
Contributor Author

@jjaffeux jjaffeux Oct 17, 2024

Choose a reason for hiding this comment

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

This is now a default, could probably use something else than EmberObject but didn't want to refactor everything

@@ -245,34 +138,6 @@ function initializeDiscoursePostEventDecorator(api) {
"notification.discourse_post_event.notifications.ongoing_event_reminder",
"calendar-day"
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move into the component

if (eventModel && eventContainer) {
eventContainer.innerHTML = "";

const datesHeight = 50;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved into the component

eventContainer.style.height = `${widgetHeight}px`;

const glueContainer = document.createElement("div");
glueContainer.innerHTML = '<div class="spinner medium"></div>';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't reimplemented the spinner logic yet, I should maybe do it

},
},
};
import { tracked } from "@glimmer/tracking";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tracked properties for events

@@ -0,0 +1,19 @@
import { tracked } from "@glimmer/tracking";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A tracked stats model

this.__type = "discourse-post-event-invitee";
},
});
import { tracked } from "@glimmer/tracking";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tracked invitee model

@@ -0,0 +1,52 @@
import { TrackedArray } from "@ember-compat/tracked-built-ins";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the invitees model; also holding suggested users

@@ -1,18 +1,23 @@
import { on } from "@ember/object/evented";
import Route from "@ember/routing/route";
import { action } from "@ember/object";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

modernizing the route

@@ -0,0 +1,126 @@
import Service from "@ember/service";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to have all the reactivity done in this file, so it's easier to know what's going on after a remote call

Comment on lines +11 to +12
put "/discourse-post-event/events/:event_id/invitees/:invitee_id" => "invitees#update"
post "/discourse-post-event/events/:event_id/invitees" => "invitees#create"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably should have avoided doing it without refactoring all of them, but I did it... No excuse.

@jjaffeux jjaffeux requested review from CvX and renato October 17, 2024 10:42
<template>
<span class="event-creator">
<a class="topic-invitee-avatar" data-user-card={{@user.username}}>
{{this.avatarImage}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend using the avatar helper:

Suggested change
{{this.avatarImage}}
{{avatar this.user imageSize="tiny"}}

}

@action
async inviteUserOrGroup(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think all these 7 methods don't need to have an event arg and could use this.args.event directly

import { cook } from "discourse/lib/text";
import i18n from "discourse-common/helpers/i18n";
import { getAbsoluteURL } from "discourse-common/lib/get-url";
import I18n from "I18n";
Copy link
Contributor

Choose a reason for hiding this comment

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

the i18n helper should be enough here

@jjaffeux jjaffeux requested review from CvX and renato October 18, 2024 10:45
Copy link
Contributor

@CvX CvX left a comment

Choose a reason for hiding this comment

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

Great stuff! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants