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

Feature/attendance viewer #319

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Andrew-Gibson42
Copy link
Contributor

Pull Request Description

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Added the event data page for tracking attendance/interest for each event. The page can be found at admin.mstacm.org/data/{urlKey}. QR/links for tracking interest/attendance have also been moved here.

NOTE: Eventually, this should be linked to from the events tool. Given the admin-web redesign, I haven't done anything with that yet, but it will at some point become necessary, as the urlKey will not be accessible from the events page once the events page is updated and the link is moved to the data page.

Copy link
Contributor

@cjwagn1 cjwagn1 left a comment

Choose a reason for hiding this comment

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

image

  • "Radio interested: Attended" doesn't make too much sense
  • Should not be a null%

Make it mobile-friendly
image

Other:

  • Change URL to "/events/data/:id" instead of the current "/data/:id". Perhaps just "events/:id" and remove the data part entirely?
  • Notify the user if it failed to fetch and when you reconnect with a little popup

Warnings:

Limit the number of warnings as best you can

  • admin-web: image
  • profile-web: image

Profile-web things:

image
What if recordInterest fails?

apps/admin-web/src/screens/EventData/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@ClayMav ClayMav left a comment

Choose a reason for hiding this comment

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

with 2 events without interest/attendance (old ones) after adding 1 attendance to the only event with an attendance code, it moved from 3rd rank to 2nd - intended behavior should be ? to 1st
if you go to a url for a nonexistent key it just kinda works like an event with no data which seems broke

Comments on carters:
I dont mind the ratio title.
null% is definitely a little strange.
Mobile friendly probably doesn't make sense here since we again use the tables from antd - like membership and mobile is not very nice for those. In the future this will need to be addressed.
events/:id definitely ideal.
failing to fetch needs to be considered.
It is a startling amount of warnings, definitely more than I am comfortable with.

I love this feature, definitely excited to see how we incorporate it into the events page and I think this will be a really killer feature for the executives. Thanks for the amazing work Andrew!

apps/admin-web/src/App.tsx Outdated Show resolved Hide resolved
Comment on lines +23 to +34
if(eventUrlKey == null) {
return (
<Modal
title={"Registration Link Error"}
visible={visible}
footer={null}
onCancel={handleCancel}
>
<h2>This event does not have a registration URL key.</h2>
</Modal>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

probably should generate one

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 will never apply to newly-created events. Only past events (before ~October or something) won't have a key.

apps/admin-web/src/screens/EventData/index.tsx Outdated Show resolved Hide resolved
apps/admin-web/src/screens/EventData/index.tsx Outdated Show resolved Hide resolved
apps/admin-web/src/screens/EventData/index.tsx Outdated Show resolved Hide resolved
apps/admin-web/src/screens/EventData/index.tsx Outdated Show resolved Hide resolved
apps/admin-web/src/screens/EventData/index.tsx Outdated Show resolved Hide resolved
@KerimD KerimD force-pushed the feature/attendance-viewer branch from aee58d7 to f59c512 Compare August 23, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants