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

In-app notification API #147

Merged
merged 43 commits into from
Jan 28, 2025
Merged

In-app notification API #147

merged 43 commits into from
Jan 28, 2025

Conversation

e32wong
Copy link
Contributor

@e32wong e32wong commented Jan 24, 2025

Changes:

  • Added support for notification API for two types of actions:
    • New opinions
    • Opinion agree/disagree
  • Implemented a new pull-down-to-refresh composable for the home feed and notification page
    • Changed the loading animation to use q-inner-loading instead since we no longer use Quasar's pull-down-to-refresh component due to it being buggy
  • Moved the new conversation page's add poll button to the top menu bar as a temporary iOS fix (we will fix it by applying a layout system that is similar to the notification page later)

@e32wong e32wong marked this pull request as ready for review January 28, 2025 06:11
@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

  • Changed the loading animation to use q-inner-loading instead since we no longer use Quasar's pull-down-to-refresh component due to it being buggy

Interesting, can you develop about what was buggy in Quasar's pull to refresh component?

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

image

The profile list of conversation is broken for some reasons. I don't know if it's related to the PR or not.

the backend does return the data, but the frontend does not display it:
image

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

Amazing work Edmund! Looks really good! Will continue the review later today.

@e32wong
Copy link
Contributor Author

e32wong commented Jan 28, 2025

  • Changed the loading animation to use q-inner-loading instead since we no longer use Quasar's pull-down-to-refresh component due to it being buggy

Interesting, can you develop about what was buggy in Quasar's pull to refresh component?

The problem started when we switched to a more standard infinite scrolling system for the home feed and notification page using VueUse's useInfiniteScroll. Infinite scrolling had been problematic because when we are using Quasar's q-layout, the page layout system has no predefined height, so it had to be changed to a fixed height div to ensure that user scrolls are detected properly. There were two options. One is to use Quasar's own infinite scrolling system, the other is to roll with useInfiniteScroll. We used to use Quasar's infinite scrolling component, don't remember why but we dropped it due to some issues. So now in this PR we are rolling with useInfiniteScroll because our own infinite scroll implementation for the front page was too complicated.

So I switched up the two pages to mount the infinite scrolling system to a full screen fixed height div. When Quasar's pull down to refresh component is mounted on a fixed height div, it caused the infinite scrolling to be laggy/jumpy, and the pull down to refresh area is also not configurable and the selection area seems to be too large when it is mounted on a div container.

@e32wong
Copy link
Contributor Author

e32wong commented Jan 28, 2025

image

The profile list of conversation is broken for some reasons. I don't know if it's related to the PR or not.

the backend does return the data, but the frontend does not display it: image

This is now fixed. It was displaying the wrong data.

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

  • Changed the loading animation to use q-inner-loading instead since we no longer use Quasar's pull-down-to-refresh component due to it being buggy

Interesting, can you develop about what was buggy in Quasar's pull to refresh component?

The problem started when we switched to a more standard infinite scrolling system for the home feed and notification page using VueUse's useInfiniteScroll. Infinite scrolling had been problematic because when we are using Quasar's q-layout, the page layout system has no predefined height, so it had to be changed to a fixed height div to ensure that user scrolls are detected properly. There were two options. One is to use Quasar's own infinite scrolling system, the other is to roll with useInfiniteScroll. We used to use Quasar's infinite scrolling component, don't remember why but we dropped it due to some issues. So now in this PR we are rolling with useInfiniteScroll because our own infinite scroll implementation for the front page was too complicated.

So I switched up the two pages to mount the infinite scrolling system to a full screen fixed height div. When Quasar's pull down to refresh component is mounted on a fixed height div, it caused the infinite scrolling to be laggy/jumpy, and the pull down to refresh area is also not configurable and the selection area seems to be too large when it is mounted on a div container.

Makes sense. I wonder what was the reason why we didn't use the infinite scrolling from quasar. It would be interesting at some point to fork the Quasar PullToRefresh component source code, and make our own custom pull to refresh component by fixing it. Because otherwise overall the look and feel was great. There are unfortunately no solid pull to refresh js library that is actively maintained.

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

  • Change <username> have disagree to your opinion to <username> have disagreeD to your opinion
  • Change <username> have agree to your opinion to <username> have agreeD to your opinion
    => going to do a bunch of changes myself

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

image

Two same conversation in the feed! with the same url (OK it happened after I deleted the back multiple times, so it may due to a weird sync error though, not priority)

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

image

keep the inside text area white (including the background of the font modifiers)

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

this should not have a grey background:

image

But a white one, like here:

image

@e32wong
Copy link
Contributor Author

e32wong commented Jan 28, 2025

image

keep the inside text area white (including the background of the font modifiers)

Fixed by adding a white background.

@e32wong
Copy link
Contributor Author

e32wong commented Jan 28, 2025

this should not have a grey background:

image

But a white one, like here:

image

Fixed with a white background.

Copy link
Member

@nicobao nicobao left a comment

Choose a reason for hiding this comment

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

I would like us to create a prettier pull-to-refresh component probably only after the stack of UI/UX change you're already doing, and after we work to release to the store

},
(t) => {
return {
userIdx: index("user_idx_notification").on(t.userId),
Copy link
Member

Choose a reason for hiding this comment

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

isn't this index already created because this is a foreign key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No PostgreSQL doesn't create indexes automatically for foreign keys.

@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

For info the rationale for renaming is:

  • simplify naming as much as possible, user seemed to be redundant here so I deleted this prefix, as I don't expect we'll have another type of notification (if we have built-in monitoring for admins/mods, we can name it differently)
  • I suggest to use "fetch" for endpoints/functions that are used to fetch possibly large amount of data (infinite scrolling etc), while we use "get" for more static data.

@nicobao nicobao merged commit b8786ec into zkorum:main Jan 28, 2025
1 check passed
@nicobao
Copy link
Member

nicobao commented Jan 28, 2025

Also for naming I suggest we use authorId when we want to say that the userId refers to the.. author of said content data (specifically for opinion/vote/conversation/response to polls...etc, meaning any data actually created by the user), while we use userId for anything else (i.e: notification generated for a specific user, static phone/passport data associated with a specific user...etc)

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.

2 participants