-
-
Notifications
You must be signed in to change notification settings - Fork 468
Feat/4545 improve news module #4579
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/4545 improve news module #4579
Conversation
onearmy-community-platform
|
||||||||||||||||||||||||||||||||||||||||||||||
| Project |
onearmy-community-platform
|
| Branch Review |
pull/4579
|
| Run status |
|
| Run duration | 24m 58s |
| Commit |
|
| Committer | othman-shamla |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
1
|
|
|
2
|
|
|
0
|
|
|
0
|
|
|
93
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
src/integration/research/read.spec.ts • 1 failed test • ci-chrome
| Test | Artifacts | |
|---|---|---|
| ... > [Visible to everyone] |
Test Replay
Screenshots
Video
|
|
profile.spec.ts • 1 flaky test • ci-chrome
| Test | Artifacts | |
|---|---|---|
| [Profile] > [By User] > [User directed to own profile] |
Test Replay
Screenshots
Video
|
|
settings.spec.ts • 1 flaky test • ci-chrome
| Test | Artifacts | |
|---|---|---|
| [Settings] > Can create member |
Test Replay
Screenshots
Video
|
|
shared/mocks/data/news.ts
Outdated
| export const news: Partial<DBNews>[] = [ | ||
| { | ||
| body: 'Test info with a link to [OneArmy](https://www.onearmy.earth/).', | ||
| body: 'Test info with a link to [OneArmy](https://www.onearmy.earth/).\n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use a firebase link. We migrated away from firebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I got that link from other test data. I used it as a temporary solution for the logo because I couldn't find a way to upload images to Supabase staging yet. The test will still pass even if I change the link to a #, as the image doesn't actually need to render for this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we still need to clean those :)
|
Heyy and thanks! Tested it out, works as expected. :)) I have one request about the zoom-in cursor on hover. In other parts of the platform (like the research, library or questions), if there is an image that is clickable to open the preview we don't use the cursor zoom-in but the classical pointer instead. So, I would also make the cursor pointer here in the news for the sake of consistency. :)) |
src/pages/News/NewsPage.tsx
Outdated
| maxWidth: '100%', | ||
| marginLeft: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these actually needed? What happens if the maxWidth and marginLeft are just removed?
|
Hi @othman-shamla, thanks so much for this! I'm afriad I'd missed previously the lightbox request(!) that us maintainers should have provided more architectural guidance on. I'll belatedly do that now if that's OK so it does extend the issue but I don't want to merge in the current approach. Again, totally the mistake of us maintainers not noticing. The main thing is that it should be a consistent approach for all images on the platform. So the new image light box should be a seperate component to do the 'useEffect', etc. and isn't on |
…ewsPage and ImageGallery
|
Apart from the zoom-in cursor comment I had above, everything seems fine, works well. :)) Thanks! |
|
Thanks @othman-shamla, this was neat contribution! @all-contributors add @othman-shamla for code |
|
I've put up a pull request to add @othman-shamla! 🎉 |
|
🎉 This PR is included in version 2.105.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the new behavior?
1. Standardized Content Width (Images & Embeds)
NewsPage.tsxfor bothimgandiframeelements to setmaxWidth: '100%'andmarginLeft: 0.2. Full-Screen Image Viewer (Lightbox)
zoom-incursor on hover.PhotoSwipe).Does this PR introduce a DB Schema Change or Migration?
Git Issues
Closes #4545