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

Improvements to the portal #3

Merged
merged 3 commits into from
Jan 13, 2022
Merged

Improvements to the portal #3

merged 3 commits into from
Jan 13, 2022

Conversation

scrnetto
Copy link

@scrnetto scrnetto commented Dec 30, 2021

Add a basic system for "verified coin profiles", e.g; social links, website, etc.
Add 'open full image' button / clickable image to the NFT Details view.
Replace "Burned NFTs" with total sales on the SC Marketplace.
Add favicon
Replace SCPscan with logo in Navbar
Show table with all downloads below the auto detection
Move Dark/Light mode switch button to right in navbar

scrnetto added 2 commits December 22, 2021 00:17
Replace "Burned NFTs" with another stat in the NFTs list of collections.

Add favicon

Replace SCPscan with logo in Navbar

Show table with all downloads below the auto detection

Move Dark/Light mode switch button to right in navbar

if (nftsSold > 0){
domTotalBurnsTxt.innerText = 'Total sold in the Marketplace';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the variables not re-named?

Copy link
Author

Choose a reason for hiding this comment

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

If you are referring to domTotalBurnsTxt, the name is always the same because the id of the container div always remains the same


function getInfoCollection(){
$.getJSON('https://stakecube.io/api/v2/marketplace/collection?contract=' + id, function(res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tbh, im not sure about this one in general. I would vote against pulling stats from a centralized third party. This makes SCPscan depending on others (even its from the same Devs) and one advantage is, SCPscan can run standalone

cc @JSKitty

Copy link
Author

Choose a reason for hiding this comment

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

We can find other values to display if there are no NFTs burned in the collection. However, if the marketplace API were no longer functional, those values would not be entered.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine to display some centralized info if it is non-mandatory data, like the Verification data is pulled for the Collections ranking, but if the request fails, the site will just fallback to other data, it won't break, a similar method here would be awesome.

Copy link
Collaborator

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

I left an in-line code review, and have two more reviews below for bugs that I don't know the origin of:

In the Download page: Sometimes executables will be tagged as "signatures", the v1.1.7 release binary was tagged as a signature, but the v1.1.6 release was tagged correctly.
image

And in the home page (Tokens), clicking on "Total Users" will popup a very dark overlay, and then the page becomes unusable: I cannot click anything without refreshing the page, seems to be some CSS issue caused by this PR.

Other than these issues, the PR seems great: the download page is awesome, logo looks nice on all pages, etc.

public/collections.html Show resolved Hide resolved
@JSKitty JSKitty self-assigned this Jan 11, 2022
@JSKitty JSKitty added the enhancement New feature or request label Jan 11, 2022
Changed image ico
Fixed modal problem
Fixed the recognition of the file type on the downloads page
@scrnetto
Copy link
Author

@JSKitty I have made the required corrections

@JSKitty
Copy link
Collaborator

JSKitty commented Jan 13, 2022

Perfect! All issues were fully corrected, I've tested all. 😄

@JSKitty JSKitty merged commit 0bd4217 into stakecube:main Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants