kodadot / nft-gallery

Generative Art Marketplace
https://kodadot.xyz
MIT License
602 stars 347 forks source link

feat: notification box #5442

Closed floyd-li closed 1 year ago

floyd-li commented 1 year ago

Thank you for your contribution to the KodaDot NFT gallery.

this pr is not completed yet, and have few questions:

  1. i'm not sure if i used the correct query. seems the events query do not support interaction = offer / accepted offer. or use another query?
  2. ~should notifications need a mark as read like other apps? if so, i think we need a new backend api.~ should we limit the notifications on this? like display the latest 10?

just tried to contact some kodadev guys on discord for their advice, but seems they are offline these days and not get feedback from them :(

πŸ‘‡ _ Let's make a quick check before the contribution.

PR Type

Context

Before submitting pull request, please make sure:

Optional

Had issue bounty label?

Community participation

Screenshot πŸ“Έ

kodabot commented 1 year ago

WARNING @floyd-li PR for issue #4890 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #4890

netlify[bot] commented 1 year ago

Deploy Preview for koda-canary ready!

Name Link
Latest commit 24098fb46f0049354ee5c5130039afb38cec2bbc
Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64310b5efc4d6600077dff6f
Deploy Preview https://deploy-preview-5442--koda-canary.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

yangwao commented 1 year ago

just tried to contact some kodadev guys on discord for their advice, but seems they are offline these days and not get feedback from them :(

it's always ask here on the issue to stay on context, I believe not all of them are always around Discord than Github, for example my case 😁

great work, it's working for me image

floyd-li commented 1 year ago

hmmmm, just talked with @roiLeo seems the notification need external service?

also currently i just displayed all events for the account. and not sure how to get the offer and acceped offer events πŸ€”οΈ

yangwao commented 1 year ago

seems the notification need external service?

it's fine, we are doing local version first to give creators overview on their sales, we have right now none πŸ˜…

external service?

will be in future.

how to get the offer and acceped offer events πŸ€”οΈ

oh I'm afraid for that we will need to have resolver? cc @vikiival

yangwao commented 1 year ago

oh wait, I think you are showing wrong events, because I see my own BUY and LIST? I should see SALE of my NFTs

image

roiLeo commented 1 year ago

seems the notification need external service?

IMO, an external database is needed to tell user which notification he has seen/deleted

also currently i just displayed all events for the account. and not sure how to get the offer and acceped offer events πŸ€”οΈ

issue reported in kodadot/snek#169

I don't see any limit in your queries, it might be a problem on client side

vikiival commented 1 year ago

Thanks for the issue!

Will try to make today/tomorrow

@yangwao as @roiLeo mentioned is more bug than resolver

exezbcz commented 1 year ago

Feedback

Functional

Visual

stacking collection

hover over event

hover effect on buttons

And those are I think the major issues

Thank you! Nice job so far πŸš€

vikiival commented 1 year ago

Hey was talking w/ @yangwao that there are higher prio issues than - kodadot/snek#169 but I would be happy if someone would fix that. May @roiLeo can try

yangwao commented 1 year ago

deployment is here https://deploy-preview-5442--koda-canary.netlify.app/

notification icon seems very dark image and very white image is there at all?

exezbcz commented 1 year ago

yeah, almost all icons in the deploy disappeared image

floyd-li commented 1 year ago

deployment is here https://deploy-preview-5442--koda-canary.netlify.app/

notification icon seems very dark image and very white image is there at all?

seems the icon got lost, will fix it later

reviewpad[bot] commented 1 year ago

AI-Generated Pull Request Summary: This pull request introduces a new notification feature in the navbar. It adds a new NotificationBoxButton component that displays notifications when clicked. The notifications are fetched using the new "collectionByAccount" and "eventListByAccount" GraphQL queries. The notifications can be filtered by collection or by event type, and the displayed events can be clicked to redirect to the corresponding gallery page. Additionally, this PR includes updates to the language files and related components to support the new feature.

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request introduces significant changes to the notification system. It adds a new NotificationBoxButton component and corresponding modal, which display notifications related to the user's activity, such as sales, offers, and accepted offers. The user can now filter notifications by collections and specific events (e.g., by sales, offers, or accepted offers). Additionally, localization texts are updated for a better user experience. Lastly, new GraphQL queries for fetching collections and event lists by an account are added.

yangwao commented 1 year ago

hey can we close this by showing those SALE events for now and follow up once resolver will be done?

floyd-li commented 1 year ago

hey can we close this by showing those SALE events for now and follow up once resolver will be done?

alright i'll close this pr and keep this branch since most of jobs are done.

if some one finish the resolver part, please let me know.

cc @vikiival

vikiival commented 1 year ago

@floyd-li I think we did not understood each other.

We DO NOT have SALE interaction because WE CALL IT BUY

Screenshot 2023-04-06 at 13 29 18

Happy to see a new PR ^-^

Screenshot 2023-04-06 at 13 28 38
yangwao commented 1 year ago

alright i'll close this pr and keep this branch since most of jobs are done.

hey @floyd-li I mean like "close" to merge without need waiting for the resolver part, so we can split it in two PRs and have this notification there and don't need wait for OFFERs event

vikiival commented 1 year ago

don't need wait for OFFERs resolver

There is no need for any resolver

yangwao commented 1 year ago

There is no need for any resolver

ah sry, mean offer event

vikiival commented 1 year ago

Yup but buy can be implemented anyway

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request includes the following changes:

These changes allow users to view their notifications in a structured and user-friendly way, provided with filters for easy access to required information.

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request mainly focuses on updating code related to RMRK 2.0, introducing a new NotificationBox component, and adding new GraphQL queries for fetching notifications by account. Several files have been added or modified, including new Vue components NotificationBoxModal.vue, NotificationBoxButton.vue, and NotificationItem.vue, as well as GraphQL files collectionByAccount.graphql and notificationsByAccount.graphql. The PR also includes updates to the chain configuration, endpoint maps, and chain-related utility functions.

Among the changes are reorderings of Prefix and BackwardPrefix types, modifications to type definitions and function arguments in various chain-related files, new helper functions, and enhanced formatting capabilities in the formatToNow function. Renamed or reorganized keys in several objects and updates to locales, particularly with regard to notifications, also form part of the PR.

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request adds a NotificationBox feature to the Navbar component, allowing users to view and manage event notifications. The NotificationBoxButton component is added to the Navbar, which when clicked, triggers a modal showing a list of notifications. Users can filter these notifications by collection and event type, and can toggle between displaying and hiding filters. The PR also introduces new GraphQL queries to fetch collections and notifications by account. Lastly, some internationalization and utility functions are added for handling notification-related text and time formatting.

floyd-li commented 1 year ago

implement of SALE, OFFER and ACCEPTED OFFER. tested on snek

cc @roiLeo @vikiival


btw is this PR okay for review?

yangwao commented 1 year ago

seems like working!

yet I sense colors are different than in design πŸ‘€

image

yangwao commented 1 year ago

btw is this PR okay for review?

I'm ok to merge how it is, if @kodadot/code-review-guild won't have any critical suggestion and fix rest stuff in following up PR :)

vikiival commented 1 year ago

btw is this PR okay for review?

I'm ok to merge how it is, if @kodadot/code-review-guild won't have any critical suggestion and fix rest stuff in following up PR :)

Yup please @floyd-li test it on RMRK & RMRk2

I think it will crash there since offers does not exist

exezbcz commented 1 year ago

yeah, spotted the same as roiloe that some events are not related to my account

some visual stuff

responsivness of the button image

ad the line in the middle of the screen:

please add colors to the tags according to the design

feel free to create follow up

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request includes changes that add a new notification feature to the navbar. A new NotificationBoxButton component is introduced, which, when clicked, opens a NotificationBoxModal displaying various filter options and a list of notifications. The notifications can be filtered by collections and events, and an empty state is handled when no matching notifications are found. Additionally, the pull request includes new GraphQL queries to fetch notification data, updates to the existing time formatting utility function, and new translations related to the notification feature.

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request adds a Notification Box feature to the Navbar. It introduces new components, such as NotificationBoxButton, NotificationBoxModal, and NotificationItem. It also adds new GraphQL queries for fetching user-specific notifications and collections. Furthermore, the pull request updates the locale file for English translations, and makes minor style adjustments to the _variables.scss file. The new components display notifications, along with their corresponding filters, in a dropdown menu on the Navbar.

exezbcz commented 1 year ago

hmm, tooltip, not sure if we should include that - I would add some delay, lets try 1s for now and then we can customize it later

image

sidebars still stack on top of each other

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request introduces a notification system for users. It adds a NotificationBoxButton component to the Navbar.vue and creates new components for the notification box: NotificationBoxModal, NotificationItem, and several utility files for handling notifications, filters, and types. The notification box displays filtered events, including sales, offers, and accepted offers for the user's account. Additionally, this PR updates the en.json localization file, adds new GraphQL queries to retrieve notifications by account, and modifies some styling in _variables.scss and time.ts.

floyd-li commented 1 year ago

@daiagi hi dude thanks for your review! just noticed that you commented lots of styles usage for it. i mainly refer the wallet connect modal since these two much alike, and used some of its styles _connect-wallet.scss i think there's lot of changes to be make for this file, and it may be refactored with lots of modals. should i make the change you mentioned now? or we wait the refactor next and modify them together? wdyt?

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request adds a notification box feature to the navbar component, allowing users to view and filter notifications related to their account. It includes the addition of a NotificationBoxButton component, a NotificationBoxModal component, and new GraphQL queries for fetching notifications data. Additionally, it adds new translations for the notification text and updates some styling variables.

daiagi commented 1 year ago

Hi I know it seems like a lot but it's small tasks each Please address them in this PR

floyd-li commented 1 year ago

Hi I know it seems like a lot but it's small tasks each Please address them in this PR

alright will do the changes.

but seems bulma only support these 6 spacing not including like 2rem things, seems we need to extend it so we can support more complex spacing. or we follow it as standard at design?

Suffix Value
*-0 0
*-1 0.25rem
*-2 0.5rem
*-3 0.75rem
*-4 1rem
*-5 1.5rem
*-6 3rem
reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request introduces a new notification feature for the navbar. It adds a notification box button component to the navbar, which when clicked, displays a notification modal with a list of events, such as sales and offers, filtered by collections and event types. Users will now be able to view notifications related to their account inside the application. The code includes new GraphQL queries to fetch the required notification data and new components for displaying the notifications. Additionally, some new styles and utility classes for the notification feature have been added.

codeclimate[bot] commented 1 year ago

Code Climate has analyzed commit 24098fb4 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

reviewpad[bot] commented 1 year ago

AI-Generated Summary: This pull request introduces a new notification system for the application. It includes:

  1. The addition of a NotificationBoxButton component in the Navbar.
  2. The creation of a NotificationBoxModal that serves as the container for the notifications.
  3. The implementation of filters (based on Collection and Event) in the notification modal.
  4. The addition of new queries related to notifications and collections.
  5. Enhancements to the global styles and language translation files.

The new features provide users with the ability to view and manage notifications related to sales, offers, and accepted offers based on the selected filters.

yangwao commented 1 year ago

nice, like it! image

image

I would like to close it πŸ‘€ image

daiagi commented 1 year ago

image

yangwao commented 1 year ago

on Basilsik I'm not seeing any query being sent when opening notifications

seems works fine for me

on Snek - getting events that are not relevant to my account on Kusama - the query fails

I have idea if you @daiagi you could handover this? As I really move forward this one :)

On one PR is already quite a lot of effort and I want to avoid big PRs like this.

yangwao commented 1 year ago

pay 100 usd

yangwao commented 1 year ago

😍 Perfect, I’ve sent the payout πŸ’΅ $100 @ 32.02 USD/KSM ~ 3.123 $KSM πŸ§— EZiu1PjV2j2JHKxY6mHnFwwCRCoV27uHKQSkKXATSh1srJT πŸ”— 0x63f0500b058726a0c43c4d763393a6156655a0cede66d4fff9108bd492ce0cbc

πŸͺ… Let’s grab another issue and get rewarded! πŸͺ„ github.com/kodadot/nft-gallery/issues