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.

Sign up for GitHub

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

Jump to bottom

[VO-710] feat: Show announcements #2177

Merged
merged 2 commits into from
Sep 3, 2024
Merged

[VO-710] feat: Show announcements #2177

merged 2 commits into from
Sep 3, 2024

Conversation

cballevre
Copy link
Member 

### ✨ Features

* Show announcements

### 🔧 Tech

* Update cozy-ui from 111.0.0 to 111.8.0
* Update cozy-flags from 3.2.2 to 4.0.0

This PR allow to displays a modal with the latest announcement from the cozy ecosystem

Related PRs :

Preview :

Capture d’écran 2024-08-30 à 17 59 13

@cballevre cballevre requested review from JF-Cozy and zatteo and removed request for doubleface, acezard and JF-Cozy August 30, 2024 16:02
@bundlemon BundleMon
Copy link

bundlemon bot commented Aug 30, 2024 •
edited
Loading

BundleMon

Files updated (7)
Status Path Size Limits
✅ app/home.(hash).js
 47.16KB (+1.71KB +3.77%) 50KB
✅ services/deleteAccounts/home.js
 309.73KB (+891B +0.28%) 500KB
✅ services/myselfFromIdenties/home.js
 234.26KB (+830B +0.35%) 245KB
✅ services/softDeleteOrRestoreAccounts/home.js
 464.31KB (+827B +0.17%) 490KB
✅ services/updateAccounts/home.js
 461.37KB (+765B +0.16%) 467KB
✅ intents/home.(hash).js
 25.51KB (+85B +0.33%) 35KB
✅ vendors/home.(hash).js
 1.5MB (-1.67KB -0.11%) 1.6MB
Unchanged files (6)
Status Path Size Limits
✅ services/polyfillFetch/home.js
 97.38KB 99KB
✅ vendors-home.(hash).(hash).min.css
 38.77KB 40KB
✅ services/attributesHelpers/home.js
 15.23KB 16KB
✅ app-home.(hash).min.css
 1.78KB 15KB
✅ intents/index.html
 649B 1KB
✅ intents-home.(hash).min.css
 158B 6KB

Total files change +3.36KB +0.1%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

Ldoppea
Ldoppea approved these changes Sep 2, 2024
View reviewed changes
Copy link
Member

@Ldoppea Ldoppea left a comment

Choose a reason for hiding this comment

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

Only one question about the fetchJSON call. Everything else is Nit.

All the logic seems good, nice work 👍

src/components/Announcements/Announcements.tsx Outdated Show resolved Hide resolved
src/hooks/useAnnouncements.tsx Outdated Show resolved Hide resolved
src/hooks/useAnnouncements.tsx Outdated Show resolved Hide resolved
src/hooks/useAnnouncements.tsx Outdated Show resolved Hide resolved
src/hooks/useAnnouncements.tsx Outdated Show resolved Hide resolved
src/hooks/useAnnouncementsSettings.tsx

const saveAnnouncement = (data: {
dismissedAt?: string
seen?: string[]
Copy link
Member

Choose a reason for hiding this comment

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

seen is optional here but not in the save declared in line 25. Is that expected?

src/components/Announcements/AnnouncementsDialog.tsx Show resolved Hide resolved
src/hooks/useAnnouncementsUploads.tsx Outdated
import { AnnouncementsConfig } from 'components/Announcements/types'
import flag from 'cozy-flags'

const useAnnouncementsUploads = (url: string | undefined): string | null => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: My understanding is that upload is a name relative to announcements but does not means that we are uploading anything here? Would be nice to have some nomenclature to explain this somewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

We are not uploading anything we only get images from the announcements server which are located inside the uploads route. To avoid misunderstanding I added some documentation and also renamed the hook to useAnnoncementsImage

src/hooks/useAnnouncementsUploads.tsx Outdated
Comment on lines 16 to 21
const fetchBinary = (await client?.stackClient.fetch(
'GET',
`/remote/${config.remoteDoctype}.uploads?url=${urlWithoutPrefix}`
)) as Response
const blob = await fetchBinary.blob()
setImage(URL.createObjectURL(blob))
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why we need to fetch the image in JS side instead of giving the URL to the <img> element?

Copy link
Member Author

Choose a reason for hiding this comment

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

I follow a similar process as for the wallpaper (code) because I had a problem with authentification and CSP

src/components/Announcements/helpers.ts Outdated Show resolved Hide resolved
@cballevre cballevre merged commit 0955d87 into master Sep 3, 2024
3 checks passed
@cballevre cballevre deleted the feat/show-announcements branch September 3, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ldoppea Ldoppea Ldoppea approved these changes

@zatteo zatteo Awaiting requested review from zatteo

@JF-Cozy JF-Cozy Awaiting requested review from JF-Cozy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cballevre @Ldoppea