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: Add announcement module #156

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

Conversation

Hardik-hi
Copy link

@Hardik-hi Hardik-hi commented Jul 4, 2022

Description

Added Announcements Module.

Changes

  1. Added filters for announcements
  2. Added infinite scroll for viewing announcements (current implementation is a mock, in actual scenario multiple announcements will load on scrolling as configured by admin)
  3. Added pinned announcements
  4. Added modal to view announcement, with custom badges and tags

Announcement_User_1 0 0_Desktop

How to test

Run the announcements and core modules using lerna run start --parallel --scope='{core,announcements}'

CC: @coolbung

Use cases

  1. Pinned announcements: announcements which appear on top of other announcements. They are sorted by decreasing order of date by default. They are of two types- dismissable and non-dismissable, the former can be closed with a X mark on the right hand side.

  2. Infinite scroll: When user scrolls in the announcements display area, some announcements are loaded in sets of admin configured value, say 20.

  3. Each announcement has a badge of announcement type shown on right hand side in the list. This can be configured from the backend.

  4. On opening each announcement, some optional tags are also there, say for an event, there may be tags showing the date, time and venue of the event. They can be used to quickly skim through the announcement details.

@coolbung
Copy link
Collaborator

coolbung commented Jul 5, 2022

Looking great @Hardik-hi Can you also update the description with a few uses for the benefit of other reviewers.

@sagarihanda @RinkleDang -- request you to take a look at the demo and share feedback on the UI. One thing I'd certainly like is for the pinned notifications (the one in the yellow background is a pinned one) sitting all the way on the top on all pages

@RinkleDang
Copy link

Hello @Hardik-hi

This good great and hindi translation is also impressive! I just had 2 inputs :

  1. We may make the yellow notification clear about what does it represent, as for now it seemed more of a tip to me until I read the user cases, so possibly some sort of an icon or a tag which makes sure that it is a pinned notification should work. Additionally, we can explore different colours (for instance, light orange being used for other notifications frame as a background here) to make sure it stands out but is aligned with the flow below. You could also explore if this should be placed down right above the whole notifications scroll as right now this seems a little in the air.

  2. Additionally, not sure if I understand the first user-case in terms of dismissable and non-dismissable. If you could help me understand it's implications, that would be great. However, as Ashwin said, pinned notifications should appear on all pages.

Thank you!

@Hardik-hi
Copy link
Author

Hi @RinkleDang. Thanks for the inputs.

  1. I will add a pinned icon to the left side to the notification. The colors are configurable from the admin portal so they can be set accordingly. Regarding the placement, the example @coolbung provided me was a banner like in (this). As per my understanding they are more of an announcement banner, should catch the user's attention as soon as they open the page (@coolbung correct me if I'm wrong) hence I have placed it separately from the other announcements; placing it above other announcements would be a bit less differentiating I believe.

  2. Some announcements/banners cannot be dismissed by the user while others can be, like in this. This too, can be configured by the admin. So the whole idea is for the admin to decide whether they want the banner to stay atop mandatorily or to not disturb the user as they use the page.

@coolbung Please provide any points which I might have missed.

Thanks

@Hardik-hi
Copy link
Author

@RinkleDang It looks like this with the pin icon:

image

@coolbung
Copy link
Collaborator

coolbung commented Jul 7, 2022

This looks good. Can you move the pinned announcements to the very top of the screen @Hardik-hi i.e. on top of the back icon. What would that need ?

cc @arajput

@Hardik-hi Hardik-hi force-pushed the features/announcements-module branch from 494d064 to 315f52e Compare July 11, 2022 08:36
@Hardik-hi
Copy link
Author

Hey @coolbung I've pushed the changes
It looks like this

image

@Shruti3004
Copy link
Member

Hey @Hardik-hi build is faliing can you please run yarn-format in root.

@arajput
Copy link
Collaborator

arajput commented Jul 12, 2022

@Hardik-hi ,
$ prettier --check .
Checking formatting...
[warn] packages/announcements/public/locales/hi/announcements.json
[warn] packages/announcements/src/pages/Announcements.js
[warn] jsxBracketSameLine is deprecated.
[warn] Code style issues found in the above file(s). Forgot to run Prettier?

please run yarn format to fix code format issues


module.exports = {
devServer: {
port: 3008,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please check if this port does not conflicts with port in other packages?

Copy link
Author

Choose a reason for hiding this comment

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

Checked, changed port to 4002

Copy link
Collaborator

Choose a reason for hiding this comment

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

keep the pot number in 3000 series only


module.exports = {
devServer: {
port: 3008,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is port used by some other modules?

Copy link
Collaborator

@arajput arajput left a comment

Choose a reason for hiding this comment

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

pls go trough the comments and update

@Hardik-hi Hardik-hi force-pushed the features/announcements-module branch from 315f52e to 3bb753c Compare July 12, 2022 18:05
@Hardik-hi
Copy link
Author

@Shruti3004 @arajput I have updated the code and pushed, please review

@@ -25,6 +25,7 @@ export default function AppBar({
onPressBackButton,
rightIcon,
isShowNotificationButton,
showPinnedAnnouncements,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this variable used?

Copy link
Collaborator

@arajput arajput left a comment

Choose a reason for hiding this comment

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

pls correct the port number too 3000 series.
fix code format issues using yarn format

@Hardik-hi Hardik-hi force-pushed the features/announcements-module branch from fdcd8e4 to 7885da0 Compare July 13, 2022 06:14
@Hardik-hi
Copy link
Author

@arajput Changed port to 3010 now, please review

@Hardik-hi Hardik-hi force-pushed the features/announcements-module branch from 7885da0 to 54e1316 Compare July 18, 2022 18:57
@Hardik-hi Hardik-hi requested a review from arajput July 20, 2022 15:36
@Hardik-hi Hardik-hi force-pushed the features/announcements-module branch from 3b577f0 to 73c8a1f Compare August 5, 2022 12:42
@Hardik-hi Hardik-hi force-pushed the features/announcements-module branch from a70b94f to 744ea8f Compare August 23, 2022 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants