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

fix: Replace home banner with news carousel. #1417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

apoorvapendse
Copy link

This pull request fixes the issue of displaying home banner instead of a news carousel on the top menu.
Fixes: #1409

Video Demo:
https://github.com/user-attachments/assets/43ac97a7-cae1-434d-8fb0-e153bdd0e060

Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for testitori ready!

Name Link
🔨 Latest commit e84363b
🔍 Latest deploy log https://app.netlify.com/sites/testitori/deploys/6743dd3637e17c0008d78000
😎 Deploy Preview https://deploy-preview-1417--testitori.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 configuration.

Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for teritori-dapp ready!

Name Link
🔨 Latest commit cb263c4
🔍 Latest deploy log https://app.netlify.com/sites/teritori-dapp/deploys/674924a043568e000823c1ac
😎 Deploy Preview https://deploy-preview-1417--teritori-dapp.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 configuration.

@apoorvapendse apoorvapendse changed the title fix(topmenu) : Replace home banner with news carousel. fix: Replace home banner with news carousel. Nov 25, 2024
@apoorvapendse apoorvapendse force-pushed the main branch 2 times, most recently from 0da5558 to 5233526 Compare November 25, 2024 12:31
Copy link

netlify bot commented Nov 25, 2024

Deploy Preview for gno-dapp ready!

Name Link
🔨 Latest commit cb263c4
🔍 Latest deploy log https://app.netlify.com/sites/gno-dapp/deploys/674924a094f84600087c001c
😎 Deploy Preview https://deploy-preview-1417--gno-dapp.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 configuration.

@WaDadidou
Copy link
Collaborator

As we don't have design for that, we expect you to purpose a design for the TopMenu version of the News carousel.
I know it's not really clear, you need more information to handle this issue.

Here is a guideline:

  • Blue: The big one is in an horizontal rectange, you must keep a horizontal rectangle shape (approximative) to the TopMenu
  • Green: Just show the title and subtitleon top left, the buttons CTAs on bottom left, and the image on right
  • Red: Don't show the text
  • Obvisouly, reduce the image height/width, the title and subtitle height/width. You reduced the title size, but not the subtitle size. The title must be bigger than the subtitle, but both must be smaller (Not too smaller, refer to the other font sizes used in TopMenu)
  • I recommend you to make a TopMeuNewsBox and use it in TopMenuHighligjtedNews.
    $image

About your design:

  • It takes too much space,
  • The buttons are too big, we already have buttons in TopMenu, use the same size
  • There are vertical spaces above and bellow
  • The left/right buttons must be at the right of the title "Highlighted News", on the same line, like the big one on the home page

image

@WaDadidou
Copy link
Collaborator

Sorry for the lack of information, we are kinda busy, but I want to test colaborating with external people.
I modified the tasks on the issue: #1409

@apoorvapendse
Copy link
Author

apoorvapendse commented Nov 26, 2024

As we don't have design for that, we expect you to purpose a design for the TopMenu version of the News carousel. I know it's not really clear, you need more information to handle this issue.

Thanks for the detailed information and feedback @WaDadidou !
I'll work on this and get back to you in a couple of days 👍

@apoorvapendse
Copy link
Author

apoorvapendse commented Nov 26, 2024

Sorry for the lack of information, we are kinda busy, but I want to test colaborating with external people. I modified the tasks on the issue: #1409

Thanks a lot for taking out the time.
I really like how you point out the relevant code lines in the issues.
Becomes really useful for people new to the codebase like myself.

@apoorvapendse
Copy link
Author

apoorvapendse commented Nov 28, 2024

Hey @WaDadidou,
I have implemented the changes suggested and would appreciate some feedback prior to updating the PR.

updates.mp4

Little confused about how to approach setting the carousel height.
Should we adjust the height of the carousel dynamically based on the news box?
I'm not entirely clear on the best way to handle this.

@WaDadidou
Copy link
Collaborator

WaDadidou commented Dec 16, 2024

Hello @apoorvapendse ,
I understand the confusion ^^"
The height must be fix.
Exemple: If a News has no subtitle, there will be an empty space. If the News has a big title or subtitle, it will be cropped (See numberOfLines props of BrandText).

Regarding your last video, you can:

  • Reduce the subtitle font size
  • Reduce the image size

Here is the layout that I think is good:
image

🟦 Blue: "Highlighted News" + Left/Right buttons horizontally
🔴 Red: Title + Subtitle (2 lines maximum for title and 2 lines maximum for subtitle)
🟨 Yellow: Image
🟢 Green: Buttons CTAs horizontally

image

Maybe around 340px as max height for the whole the could be good.


Thank you for your exploration, please try to integrate this layout and push your code :)
I take care about this issue, and I train myself in communication

@apoorvapendse
Copy link
Author

Thank you for your exploration, please try to integrate this layout and push your code :) I take care about this issue, and I train myself in communication

Thanks for the detailed reply, I'll update the PR soon 👍

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.

fix(top-menu): Replace the banner by the news
2 participants