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

chore: fetch personal pp and remove notifications #23

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Mar 3, 2024

Resolves #21

  • failing to fetch the org pp will fetch from the users endpoint
  • added NOTIFICATION_EXPIRY_DAYS = 3
  • it seemed that previously the isNew prop was being wiped after it experienced it's first cache refresh, this way it's based on the created_at which seems like a better approach

The timeframe looks a little squished when it's at defcon 1 don't you think?

image

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Mar 4, 2024

@0x4007
Copy link
Member

0x4007 commented Mar 4, 2024

Let's make it simpler. Can we just remove the new issue indicator? The CSS and TS code for it.

I think in order to properly implement it, the caching needs to be rebuilt entirely, because I poorly implemented the caching the first time around.

The timeframe looks a little squished when it's at defcon 1 don't you think?

I was thinking the same. Do text-overflow: ellipsis; overflow: hidden; display: inline-block; on all labels. Test on mobile and desktop to make sure that inline-block doesn't screw anything up.

it seemed that previously the isNew prop was being wiped after it experienced it's first cache refresh, this way it's based on the created_at which seems like a better approach

No the intent is to ensure that its new for the specific user viewing on the client. Not if its a new issue for the network. Its supposed to act like a social media notification in a sense. Or like a GitHub notification.

@Keyrxng Keyrxng requested a review from 0x4007 as a code owner March 4, 2024 10:09
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Mar 4, 2024

I understand the notifications now, the vid shows them working as described. I delete them from my local storage to show it twice. Would you rather I just remove it all completely?

notifis.mp4

mobile_inline_block

desktop_inline_block

@Keyrxng Keyrxng changed the title chore: fetch personal pp and add 3 day new issue notifi chore: fetch personal pp and remove notifications Mar 5, 2024
@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

I just checked again on my computer and see that CI failed. Just some minor cleanup please

https://github.com/ubiquity/work.ubq.fi/actions/runs/8162423256/job/22318480984#step:4:93

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

Strange, the error did not appear in the latest commit deployment for me when I tried in my browser. @FernandVEYRIER any ideas why this would be? I tried incognito as well.

@0x4007 0x4007 merged commit 7f35ce4 into ubiquity:development Mar 6, 2024
1 of 2 checks passed
@ubiquibot ubiquibot bot mentioned this pull request Mar 6, 2024
@gentlementlegen
Copy link
Member

Not sure, can have a look if that error persist. You can always check the artifacts as well within the Action, like here
https://github.com/ubiquity/work.ubq.fi/actions/runs/8162423256
According to the screenshot the div indeed does not have the new-task styling, somehow

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

Yes I noticed the screenshot as well. Was curious if there's benefit to doing videos as well, as I saw that as an option in the config.

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

Is it possible that Cypress is pulling in the wrong commit? This seems the most likely. However the first deployment doesn't even seem stable.

@gentlementlegen
Copy link
Member

Sure I can enable the video, I didn't because it uses a lot of storage and I didn't know about the current limitations. I don't think Cypress would pull the wrong commit, will look into it

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

I didn't know about the current limitations

Since we're open source, GitHub sort of gives us unlimited storage etc.

@0x4007
Copy link
Member

0x4007 commented Mar 6, 2024

@FernandVEYRIER I just realized you wrote custom tests for this UI. So this makes sense. You specifically requested that it checks for new-task which was removed here.

@0x4007 0x4007 mentioned this pull request Mar 6, 2024
ghost pushed a commit to ariesgun/work.ubq.fi that referenced this pull request Oct 13, 2024
use `eslint-plugin-filename-rules`
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.

March Adjustments
3 participants