-
Notifications
You must be signed in to change notification settings - Fork 1
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
109 agenda session registrations #125
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, if you find it useful, I can go back to refactor some code (as suggested in the comments) for more stability and clarity, between Wednesday and Sunday. Or I can work on other stuff, as you prefer :)
@@ -7,6 +7,7 @@ import { DynamoDB, HandledError, ResourceController } from 'idea-aws'; | |||
import { Session } from '../models/session.model'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I'd rename this RC "sessionsRegistrations", not to be confused with users registrations
const date = new Date(dateToFormat); | ||
return new Date(date.getTime() - date.getTimezoneOffset() * 60000).toISOString().slice(0, 16); | ||
return new Date( | ||
date.getTime() - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the merge I can come back to this timezone logic and simplify it, similarly to how we do in the GA app
@@ -50,6 +55,10 @@ export class Speaker extends Resource { | |||
typeof x.organization === 'string' | |||
? new OrganizationLinked({ organizationId: x.organization }) | |||
: new OrganizationLinked(x.organization); | |||
this.socialMedia = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the merge I can come back with a better structure for social media, if you want (copying Papaya)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be part of socialMedia
@@ -95,6 +99,10 @@ export class User extends Resource { | |||
this.registrationForm = x.registrationForm ?? {}; | |||
if (x.registrationAt) this.registrationAt = this.clean(x.registrationAt, t => new Date(t).toISOString()); | |||
if (x.spot) this.spot = new EventSpotAttached(x.spot); | |||
this.socialMedia = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the merge I can come back with a better structure for social media, if you want (copying Papaya)
@@ -208,4 +208,13 @@ export class AppService { | |||
const p = this.user.permissions; | |||
return p.isAdmin || p.canManageRegistrations || p.canManageContents; | |||
} | |||
|
|||
formatDateShort = (date: string | Date): string => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd suggest using pipes/have a logic more similar to GA app; I can come back to this if you want
… block on loading
@uatisdeproblem from your review I have resolved those that have been fixed in recent commits. The other comments I agree with them but maybe it is better to make them after the event so as not to risk breaking anything. At least those related to data. I created the issue #126 to keep track of these. If instead, you feel that there is no risk and they are easily doable in the next issues (the pipes for example). feel free to do it, just let me know so I can resolve the comments here :) Thank you a lot for your help! |
closes #109
closes #111
relates to #78
Once merged we will do some graphical improvements and bug fixing if required and then we can merge into prod.
things to improve (this PR or a new issue):
some @todos in the sessions page (i.e. we need to open the detail in a modal for mobile) + when pressing a button it automatically selects the session (ev.preventDefault() not working for some reason...). + other minor things.
Some graphical improvements can be made to most lists (both entity lists as well as the lists inside entity details).
session/sessionId
pageto ask: