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

feat: notification box #5442

Merged
merged 74 commits into from
Apr 9, 2023
Merged

feat: notification box #5442

merged 74 commits into from
Apr 9, 2023

Conversation

floyd-li
Copy link
Member

@floyd-li floyd-li commented Mar 31, 2023

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

  • Bugfix
  • Feature
  • Refactoring

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </bsx/collection>
  • I've tested PR on mobile
  • I've written unit tests 🧪
  • I've found edge cases

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

image

image

image

image

@kodabot
Copy link
Collaborator

kodabot commented Mar 31, 2023

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
Copy link

netlify bot commented Mar 31, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 24098fb
🔍 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
Copy link
Member

yangwao commented Mar 31, 2023

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
Copy link
Member Author

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
Copy link
Member

yangwao commented Mar 31, 2023

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
Copy link
Member

yangwao commented Mar 31, 2023

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
Copy link
Contributor

roiLeo commented Mar 31, 2023

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
Copy link
Member

vikiival commented Apr 2, 2023

Thanks for the issue!

Will try to make today/tomorrow

@yangwao as @roiLeo mentioned is more bug than resolver

@exezbcz
Copy link
Member

exezbcz commented Apr 2, 2023

Feedback

Functional

  • so as was already mentioned - we will display only:

    • incoming offer from someone on your nft
    • sale of your nft
    • accepted offer - So you basically bought the nft
  • user will have option to only select only one collection at a time

  • notifications dont refresh when user change network

  • also it should not stack with the wallet sidebar - only one sidebar opened at one time please

Visual

stacking collection

  • not using horizontal scroll
    image

hover over event

  • bg change
    image

hover effect on buttons

  • my bad I did not mention this

  • please match the grey stroke buttons to this style:
    image

  • they are the same as buttons in gallery item - activity section - for darkmode, see the button there please

  • these buttons work as breadcrumbs in explore/collection detail
    image

  • then change is only that they are rounded
    image

  • the add/done button is correct, good job

And those are I think the major issues

Thank you! Nice job so far 🚀

@vikiival
Copy link
Member

vikiival commented Apr 3, 2023

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
Copy link
Member

yangwao commented Apr 3, 2023

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
Copy link
Member

exezbcz commented Apr 3, 2023

yeah, almost all icons in the deploy disappeared
image

@floyd-li
Copy link
Member Author

floyd-li commented Apr 3, 2023

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
Copy link
Contributor

reviewpad bot commented Apr 7, 2023

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
Copy link
Member

exezbcz commented Apr 7, 2023

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

  • make the activation are smaller
  • I would also change the text to normal, not bold - it will look better

image

  • this button also rounded please

sidebars still stack on top of each other

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

Now white line is shown on wallet sidebar not sure if it's related (#5552)
Capture d’écran 2023-04-07 à 4 41 55 PM

Tested on /snek/gallery/331660682-7, I can see 4 "offer" events in NotificationBox but only 2 row in Offers tab maybe remove expired?
Capture d’écran 2023-04-07 à 4 42 38 PM

return colorMap[key]
}

export const useNotification = (account: string) => {
Copy link

Choose a reason for hiding this comment

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

Function useNotification has 46 lines of code (exceeds 25 allowed). Consider refactoring.

@reviewpad
Copy link
Contributor

reviewpad bot commented Apr 7, 2023

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.

components/common/NotificationBox/NotificationBoxModal.vue Outdated Show resolved Hide resolved
components/common/NotificationBox/NotificationBoxModal.vue Outdated Show resolved Hide resolved
components/common/NotificationBox/NotificationBoxModal.vue Outdated Show resolved Hide resolved
components/common/NotificationBox/NotificationBoxModal.vue Outdated Show resolved Hide resolved
@import '@/styles/abstracts/variables';

.notify-item {
padding: 0.75rem 2rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

use bulma classes please

components/common/NotificationBox/types.ts Outdated Show resolved Hide resolved
components/navbar/NotificationBoxButton.vue Outdated Show resolved Hide resolved
@floyd-li
Copy link
Member Author

floyd-li commented Apr 7, 2023

@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
Copy link
Contributor

reviewpad bot commented Apr 7, 2023

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
Copy link
Contributor

daiagi commented Apr 7, 2023

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

@floyd-li
Copy link
Member Author

floyd-li commented Apr 8, 2023

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
Copy link
Contributor

reviewpad bot commented Apr 8, 2023

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
Copy link

codeclimate bot commented Apr 8, 2023

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

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@reviewpad
Copy link
Contributor

reviewpad bot commented Apr 8, 2023

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
Copy link
Member

yangwao commented Apr 8, 2023

nice, like it!
image

image

I would like to close it 👀
image

@daiagi
Copy link
Contributor

daiagi commented Apr 9, 2023

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

  • on Kusama - the query fails
    image

  • on Snek - getting events that are not relevant to my account

image

@yangwao
Copy link
Member

yangwao commented Apr 9, 2023

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 yangwao mentioned this pull request Apr 9, 2023
3 tasks
@yangwao
Copy link
Member

yangwao commented Apr 9, 2023

pay 100 usd

@yangwao
Copy link
Member

yangwao commented Apr 9, 2023

😍 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

@yangwao yangwao added the paid pull-request has been paid label Apr 9, 2023
@yangwao yangwao merged commit 2e4fd7d into kodadot:main Apr 9, 2023
@floyd-li floyd-li deleted the feat/notification-box branch April 21, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large paid pull-request has been paid waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple notification screen
10 participants