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

Feature / Homepage banners added #542

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Conversation

ismailToyran
Copy link
Contributor

Project organization

Description

By using dashboard, dashboard user can now add banner(s) to the homepage.

Checklist

  • Base branch of the PR is dev

@ismailToyran ismailToyran self-assigned this Jan 26, 2024
@EmmanuelDrouin
Copy link
Member

One feedback on the implementation, can we have a skeleton effect while loading the background image?

  • Currently, if the image is a bit heavy, it is pretty slow to load on my side (images from 3MB are slow)
  • in the mean-time the image is loading, only the text and button are displayed. If you have a white background on your platform and set your text color to white, then it looks buggy as nothing shows up for a few seconds on the screen

Adding a skeleton loader for the component will improve the UX and remove the bug feeling for the end users

@EmmanuelDrouin
Copy link
Member

One additional point we could change maybe right now for consistency is to rename the section from “Starter kit configuration” to “Marketplace template configuration” as we stop using the “starter kit” wording.

@ismailToyran
Copy link
Contributor Author

@antho1404 have any idea how we can improve the loading of the image?
As we upload them on admin, we get IPFS link. And that's why it takes blank screen for a while.
After first time use this shouldn't be issue.

Also we don't have this loader on token cards either, same loading strategy. But we have background color for this case.
So I've added background + priority tag to the banner images.

Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

I think the priority + background should be enough.
Just one thing to fix to ensure existing platforms will not crash and then we are good to merge

@antho1404 antho1404 added this pull request to the merge queue Jan 31, 2024
Merged via the queue into dev with commit ce79985 Jan 31, 2024
2 checks passed
@antho1404 antho1404 deleted the feature/homepage-banners-added branch January 31, 2024 08:02
@ismailToyran ismailToyran mentioned this pull request Feb 8, 2024
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.

3 participants