Collapsing notifications | Modrinth | Page 1

full blaze Sep 18, 2022, 3:21 PM

#

Thread about development of collapsing notifications

stable rockBOT Sep 18, 2022, 3:21 PM

#

Welcome to your thread, @full blaze
Once you're finished, use /thread archive to close it. If you want to change the thread name, use /thread rename to do so.

full blaze Sep 18, 2022, 3:21 PM

#

lol

#

anyways

#

what's the vue way to do this? I want to have an attribute on each notification element and be able to access it somehow

#

using it for the expand/collapse button and it's kinda yuck

glacial cosmos Sep 18, 2022, 3:48 PM

#

:attr="booleanVariable"

#

also look into https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-expanded to make that collapsible element more accessible for screenreader and keyboard users

aria-expanded - Accessibility | MDN

The aria-expanded attribute is set on an element to indicate if a control is expanded or collapsed, and whether or not its child elements are displayed or hidden.

full blaze Sep 18, 2022, 3:52 PM

#

thanks, I found a solution that uses a data attribute sthn like this so I'm gonna try that and then see your suggestion

full blaze Sep 18, 2022, 4:11 PM

#

no ID or attributes used luckily, the code seems a bit cleaner

#

could someone send a sample json of a team invite notification?

#

because I'd imagine there'd be no point in collapsing them and they need to be handled differently

#

Also I think I'm done for today, I pushed my changes so far to my fork (devBoi76/knossos) on github

I'm wondering whether I want to rework the structure a bit more, for example separate UpdateNotification and TeamInviteNotification into separate ui components

full blaze Sep 19, 2022, 4:26 PM

#

nice and clean tiny_potato

#

for team invites I'm gonna reuse what was there previously because idk how it's supposed to look

full blaze Sep 19, 2022, 5:14 PM

#

can someone explain why this might be happening?

#

@glacial cosmos sorry for pinging but I'm incredibly annoyed lol

#

maybe you'll be able to help

glacial cosmos Sep 19, 2022, 5:17 PM

#

perhaps that set is empty on initialisation

#

notificationsGrouped.get(notificationMeta) returns undefined and you try to read property '0' from it

full blaze Sep 19, 2022, 5:18 PM

#

oooh, so in that moment when it doesn't display anything everything's waiting for the notifications to fetch and stuff is undefined/empty?

glacial cosmos Sep 19, 2022, 5:18 PM

#

ye most probably

full blaze Sep 19, 2022, 5:18 PM

#

hmm

#

if it's empty then why is this v-for even triggering?

glacial cosmos Sep 19, 2022, 5:21 PM

#

maybe it has keys in notificationsGrouped but no value under those keys

full blaze Sep 19, 2022, 5:21 PM

#

beacuse notificationsGrouped should never be undefined, it returns an empty Map by default

full blaze Sep 19, 2022, 5:21 PM

#

glacial cosmos maybe it has keys in `notificationsGrouped` but no value under those keys

no, I'm 100% sure it does thanks to console.log lol

#

it's totally a case of sthn being undefined though, because I was getting a similar error and had to make a default empty array for project update notifs

glacial cosmos Sep 19, 2022, 5:27 PM

#

what if you tried {{ [notificationMeta, notificationsGrouped.get(notificationMeta)] }}

full blaze Sep 19, 2022, 5:31 PM

#

🤨

#

it flashes this really quickly

glacial cosmos Sep 19, 2022, 5:32 PM

#

oh is that a weakmap

full blaze Sep 19, 2022, 5:33 PM

#

wdym weakmap?

glacial cosmos Sep 19, 2022, 5:33 PM

#

oh wait nvm you'd not be able to iterate by keys then with weakmap

#

what is notificationsGrouped type even

#

TIL you can use object as keys in normal maps :O

full blaze Sep 19, 2022, 5:36 PM

#

it's a computed value, which takes all notifications ( notificationsUngrouped, also computed), first sorts them into a map based on their title (only real way to group project updates lol): notif.title -> [notifs with same title], then for each key it puts it into a different map, but with metadata about the number of grouped notifications and some other stuff as the key

#

you know what, I'm just gonna send the code lol

#

notificationsGrouped() {
      const grouped = new Map()

      const keyToMetadata = new Map()

      for (const notification of this.notificationsUngrouped) {
        const a = grouped.get(notification.title) || []
        console.log('notif')
        console.log(notification)
        if (notification.type === 'project_update') {
          notification.version = notification.text.match(
            /The project, .*, has released a new version: (.*)/m
          )[1]
        }
        a.push(notification)
        grouped.set(notification.title, a)
      }
      console.log(grouped)

      const withMetadata = new Map()
      const groupedArray = []

      grouped.forEach((val, key) => {
        console.log('Val')
        console.log(val[0])
        if (!keyToMetadata.has(key)) {
          let name = ''
          if (val[0].type === 'project_update') {
            name = key.match(/\*\*(.*)\*\* has been updated!/m)[1]
          }
          const meta = {
            type: val[0].type,
            projectName: name,
            count: grouped.get(key).length,
          }
          if (val[0].type === 'project_update') {
            // meta.data = val
          } else if (val[0].type === 'team_invite') {
            // meta.data = val[0]
          }
          keyToMetadata.set(key, meta)
        }
        groupedArray.push(keyToMetadata.get(key))
        withMetadata.set(keyToMetadata.get(key), grouped.get(key))
      })
      console.log('with metadata')
      console.log(withMetadata)

      return withMetadata
    },

#

ignore groupedArray, I was just trying to do sthn

#

I think I'll to try to make it a "normal" array instead of this map because it's getting a bit janky

#

FINALLY

#

I changed it so notificationsGrouped is an array structured like so:

[ 
 {
  projectName: "...",
  count: number of groups,
  type: notification type,
  data: relevant data for notification type
  },
...
]

full blaze Sep 19, 2022, 6:05 PM

#

ok, now time to test with actual notifications to see if I didn't break anything

full blaze Sep 19, 2022, 6:27 PM

#

it works! I wanted to test dismissing notifs/dismiss all, I think the feature might be ready now!

#

ight, I think I'm ready to PR

wraith steeple Sep 19, 2022, 6:37 PM

#

tiny_pumpkin

worthy ore Sep 19, 2022, 7:43 PM

#

full blaze it works! I wanted to test dismissing notifs/dismiss all, I think the feature mi...

can you replace add 1 more with show 1 more?

#

or simply expand

full blaze Sep 19, 2022, 7:45 PM

#

Add 1 more?

worthy ore Sep 19, 2022, 7:46 PM

#

yeah, the button that shows the rest

worthy ore Sep 19, 2022, 7:46 PM

#

full blaze FINALLY

also replied on the wrong img, was supposed to be this one

full blaze Sep 19, 2022, 7:47 PM

#

OH

#

lmao

#

Sure

#

Btw "Collapse" is written with two ls, right?

worthy ore Sep 19, 2022, 7:48 PM

#

yeah

worthy ore Sep 19, 2022, 7:49 PM

#

full blaze it works! I wanted to test dismissing notifs/dismiss all, I think the feature mi...

hmm, for single updates, this takes a lot of space, but even if that doesn't change (can't think of a good way that doesn't bassically go back to what there was before if only 1 version is new, which I wouldn't be fully against), it's weird to have both a dismiss and dismiss all on a card where that does the same thing.

Also, besides the buttons, it's not obvious what's clickable rn. For clickable text, probably best to underline them

full blaze Sep 19, 2022, 7:50 PM

#

Sure, I could underline the version links

#

and make the "dismiss all" only appear when there is more than 1 new version

worthy ore Sep 19, 2022, 7:51 PM

#

Is there a difference between the time released and the time notified?

full blaze Sep 19, 2022, 7:52 PM

#

no afaik, the previous working was like that so I kept it

worthy ore Sep 19, 2022, 7:52 PM

#

If there is no difference, then "Released" makes more sense here imo

#

Does the more precise time/date show when you hover over time?

full blaze Sep 19, 2022, 7:54 PM

#

should it still be "notified" for team invites then?

worthy ore Sep 19, 2022, 7:54 PM

#

I'd say Invited or Sent a day ago personally

full blaze Sep 19, 2022, 7:55 PM

#

👍

#

I tried removing the divider if there's only one notification but idk about that

worthy ore Sep 19, 2022, 7:57 PM

#

if there's no divider, would probably make sense to shift the text to the right so it's not under the icon

full blaze Sep 19, 2022, 7:57 PM

#

yeah

#

ngl, that would be quite janky to implement, keep in mind that it'd also have to behave differently on mobile

#

I think I'm gonna keep the divider

#

#

also maybe the title in green should link to the mod's page

#

ok, its a link now. Should it be underlined modrinthinking

worthy ore Sep 19, 2022, 8:05 PM

#

Spacing does seem to be a bit inconsistent though:
(The exact numbers aren't important, but you can still see the difference)

#

full blaze Sep 19, 2022, 8:06 PM

#

yup, you're right, I can't unnotice it now lmao

worthy ore Sep 19, 2022, 8:07 PM

#

full blaze ok, its a link now. Should it be underlined <:modrinthinking:978821392072052836>

Yeah

#

Also, how would it look if the collapse/expand button was on the same row as the dismis all? Would save a fair amount of room, and it wouldn't be too different from the rest of site's "buttons are at the top" style

full blaze Sep 19, 2022, 8:11 PM

#

fixed the padding

glacial cosmos Sep 19, 2022, 8:12 PM

#

if it's updated 1 time how come there are two versions

full blaze Sep 19, 2022, 8:12 PM

#

I manually duplicated the node to see how it looks with more than 1 version lol

#

dw it's not a bug

glacial cosmos Sep 19, 2022, 8:13 PM

#

tiny_pumpkin

worthy ore Sep 19, 2022, 8:16 PM

#

looks like the top padding is a bit smaller than the bottom. I would say maybe the difference is cause of the screenshot, but both top and both bottoms are basically the same. (Could still just be that it's a screenshot though), would it be possible to change the bottom margin/padding to match the top ones?

full blaze Sep 19, 2022, 8:21 PM

#

#

as close as ur gonna get

#

lol

#

fixed padding in more button

full blaze Sep 19, 2022, 8:31 PM

#

worthy ore Also, how would it look if the collapse/expand button was on the same row as the...

it looks kinda bad imo, there'd need to be some funky css to make it the same height, and it would have to be at the bottom for mobile layout either way

#

and I feel having it at the bottom is more intuitive

worthy ore Sep 19, 2022, 8:32 PM

#

full blaze it looks kinda bad imo, there'd need to be some funky css to make it the same he...

It could have the /\ or / icon, that would make it the same height

Mobile makes sense yeah

worthy ore Sep 19, 2022, 8:32 PM

#

full blaze and I feel having it at the bottom is more intuitive

I guess, but that could be said about the entire site. I still forget that it isn't at the bottom like most sites

full blaze Sep 19, 2022, 8:33 PM

#

where else do you have a "show more" button at the top? modrinthinking

worthy ore Sep 19, 2022, 8:34 PM

#

I was referring to stuff like "send for review", "save as draft" and "save" in settings

full blaze Sep 19, 2022, 8:35 PM

#

aah

worthy ore Sep 19, 2022, 8:35 PM

#

But, (not usually as a normal button), a lot of expandable things on sites/apps the expanding clickable is at the top, like markdown or Q and As

full blaze Sep 19, 2022, 8:38 PM

#

I get what you mean but I still think that having our expand button at the bottom looks better. It's also not going to get mixed up with the "Dismiss all" button which I could see happening if it was at the top

worthy ore Sep 19, 2022, 8:38 PM

#

yeah

full blaze Sep 19, 2022, 8:38 PM

#

this is a huge improvement either way over the old notif design lol

worthy ore Sep 19, 2022, 8:38 PM

#

definitly

full blaze Sep 19, 2022, 8:42 PM

#

ight, pushed the changes

#

If there are any changes you want to make, suggest them now bc I'm busy tomorrow

full blaze Nov 13, 2022, 6:39 PM

#

I noticed that with the recent changes my PR has some merge conflicts. I dont think they're major ones luckily, but just to be sure, what would be the best way to resolve them? Just merge the new changes into my repo and resolve the conflicts there?

glacial cosmos Nov 13, 2022, 7:00 PM

#

just do an interactive rebase on upstream/master and drop any commits that aren't yours and came from non-squashed rewrite-parity branch (first n commits), git will stop on each commit that has conflicts that you can resolve and move onto next the commit that has conflicts until everything is rebased

#

you can create a backup branch to make sure you don't mess up anything

full blaze Nov 13, 2022, 7:04 PM

#

Thanks, I'll get around to doing that some time this week 👍

full blaze Nov 15, 2022, 7:24 AM

#

I feel like that would be out of scope for this PR + idk much about how the settings on mr work

versed cairn Nov 15, 2022, 2:13 PM

#

Hey, anything I can do on this PR?

full blaze Nov 17, 2022, 5:53 PM

#

~~ok, just want to make sure I don't mess up the git commit:
the rewrite-parity PR commit is just one giant squashed commit, so I can't really pick the ones changing notifications.vue and drop the others. I rebased the upstream changes onto a branch, resolved the conflict and modified notifications.vue enough to work properly with the new update. The issue is that the commit would have 8k changes from rewrite-parity.

If I now copy the correct file, and inside the github web conflict resolution I paste in the working file nothing bad should happen - correct?~~

full blaze Nov 17, 2022, 6:44 PM

#

ay, I figured it out! I'll change some more things to fix the design visually but the conflicts are resolved

worthy ore Nov 17, 2022, 6:54 PM

#

Out of curiosity, could there be any reason for someone to dismiss only 1 of the notifications on the group? Because if not, replacing the dismiss all with just dismiss, and removing the dismiss buttons, could have it take even less vertical space

full blaze Nov 17, 2022, 6:57 PM

#

Yeah, it would take less vertical space, I kept in order to not remove existing functionality, but I don't really see ppl using that

#

btw should I remove the refresh icon next to each notification? It takes up quite a bit of space which is very noticable on the mobile layout

#

The icon is removed in the rewrite too btw

gusty prism Nov 17, 2022, 7:01 PM

#

yeah

#

at some point notifications will be redone, the system is so so awful right now

full blaze Nov 17, 2022, 7:02 PM

#

yeah, and the notif api needs a bit of a redesign lol

gusty prism Nov 17, 2022, 7:02 PM

#

when that happens we can make them much nicer since it won't have all kinds of hardcoded strings and stuff

full blaze Nov 17, 2022, 7:02 PM

#

Currently I'm grouping them by the notification title because that's the only unique identifier lmao

#

much better

full blaze Nov 17, 2022, 7:28 PM

#

ight, I think I'm done with the changes

glacial cosmos Nov 17, 2022, 7:37 PM

#

the dismiss buttons are terribly misaligned with text stuff:P
perhaps the single notification rows should be a flex that vertically centers everything

full blaze Nov 17, 2022, 7:42 PM

#

yeah, that's better

full blaze Nov 17, 2022, 8:05 PM

#

If nobody has any more suggestions then I'd say the changes are done 👍

gusty prism Nov 17, 2022, 8:07 PM

#

I'll probably give it a design pass to bring it in line with the site's design before merging

#

But good work 👍

full blaze Nov 17, 2022, 8:07 PM

#

🎉

worthy ore Nov 17, 2022, 9:07 PM

#

It's funny that the icons have come from circle. From not being there to being added to quickly be able to tell the types of notifications apart, from removing them because they take so much space and 90% of people only get that 1 anyways

full blaze Nov 17, 2022, 9:10 PM

#

I mean, now the notification designs are more distinct, so it would be easier to distinguish them at a glance imo

#

What's cool is that now it will be much easier to add new notification types, since each is its own component

worthy ore Nov 17, 2022, 9:13 PM

#

full blaze If nobody has any more suggestions then I'd say the changes are done 👍

The only non-button related thing I can think of is maybe reducing the amount of versions shown when collapsed, as from the look of it, a mod with 3 version notifications is about the same height as 3 normal notifications.

Since the title also links, I also wouldn't be against the collapsed state showing 0 versions until expanded, but that's a very big change 😛

full blaze Nov 17, 2022, 9:14 PM

#

worthy ore The only non-button related thing I can think of is maybe reducing the amount of...

I think there's literally a single variable to change how many notifs are shown by default actually

#

maxVersions can be passed in

worthy ore Nov 17, 2022, 9:16 PM

#

worthy ore The only non-button related thing I can think of is maybe reducing the amount of...

Another advantage to having 0 versions show without expanding would you could put the expand button on the same row as everything else, so unless expanded, it would be the same height as the old notification.

Though, that's a lot more work than just reducing the number 😛

versed cairn Nov 18, 2022, 5:06 AM

#

idk if that’s just Safari, but the teaminvites look weird

full blaze Nov 20, 2022, 6:18 PM

#

versed cairn idk if that’s just Safari, but the teaminvites look weird

Yeah, tbh I kinda forgot about the team invite notification. I fixed the general layout but I have no idea whether the title is supposed to be a link or not since I never actually got a notif like that and I couldn't find an example json

versed cairn Nov 20, 2022, 6:19 PM

#

Yes, it is a link.. to the Project Page

full blaze Nov 20, 2022, 6:19 PM

#

thanks, I guess the underline and green color makes sense then

worthy ore Nov 20, 2022, 6:27 PM

#

Wait, single notifications don't use the "normal" format?

full blaze Nov 20, 2022, 6:27 PM

#

wdym by "normal"

worthy ore Nov 20, 2022, 6:28 PM

#

The old one / team invites

full blaze Nov 20, 2022, 6:29 PM

#

idk, I don't think I changed any font

worthy ore Nov 20, 2022, 6:29 PM

#

Font?

full blaze Nov 20, 2022, 6:29 PM

#

🤦‍♂️

#

I read "format" as "font"

worthy ore Nov 20, 2022, 6:30 PM

#

Oh lol

#

If it uses the "list" format for single notifications as well, then if people follow a lot of projects (that don't double/triple release), then it greatly increases the space it takes up, which feels counter to the reason for grouping them in the first place

full blaze Nov 20, 2022, 6:32 PM

#

true, so would you say using the team invite format is better in that case?

worthy ore Nov 20, 2022, 6:32 PM

#

Yeah

full blaze Nov 20, 2022, 6:39 PM

#

a much easier solution would be just removing the divider modrinthinking

#

since it's not really needed in a case like this

#

although on second though I think I'll just use the other design

worthy ore Nov 20, 2022, 6:43 PM

#

Btw, does your thing group all updates of a project info one, or only consecutive?

Like if A updated
Then B updated
Then A updated

Is A one or two cards?

full blaze Nov 20, 2022, 6:43 PM

#

yes

#

and in the list it should be sorted by the order the notifications came in (by the release date)

#

centered those buttons bc it's been driving me nuts lmao

#Collapsing notifications