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

Add `show-asset-download-count` feature #2027

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dotconnor
Copy link
Contributor

commented May 12, 2019

This only displays the count when a specific release page:
image

I don't think it would be really useful on the main releases page unless you just get the latest release's stats.

Tested on:

Closes #670

@dotconnor dotconnor changed the title feat: add download count to assets on release pages Add `show-asset-download-count` feature May 12, 2019

dotconnor added some commits May 12, 2019

Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated
Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated

const tags = select.all('svg.octicon-tag ~ span').map(tag => tag.innerText);
const tagAssets = await getAssetsForTag(tags);
for (const release of select.all('.release-entry:not(.release-timeline-tags)')) {

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

Instead of selecting again, it may be best to reuse the existing tags to ensure they point to the same stuff.

This comment has been minimized.

Copy link
@dotconnor

dotconnor May 13, 2019

Author Contributor

I'm not quite sure I'm following

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

On line 37 and 39 you’re selecting tags and releases, once to get the tag name and the second time to fill the asset list.

You can do two things:

  • select the releases at the beginning, and not the tags
  • reuse the same array to reach the asset list of each release

Also it may be worth to select only the releases that have extra assets and then avoid the query altogether if there aren’t any.

I don’t know if I’m being clear enough 😬

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

What makes the current implementation “wrong” is that, for example, you’re selecting the tag 3 times in the code (svg.octicon-tag ~ span)

An example solution:

tags = select.all()
downloadCounts = await api(tags)
for tag of tags // reuse `tags` instead of using select.all again
  Add downloadCounts[tag] to tag
endfor

This comment has been minimized.

Copy link
@dotconnor

dotconnor May 13, 2019

Author Contributor

Given the dom layout, I still don't think you can select just the tags once:

  Release
     |
 |--------|
Tag     Assets

If you select the tags only once, then you lose your only connection to the assets (releases).

tags = select.all()
downloadCounts = await api(tags)
for tag of tags
  Add downloadCounts[tag] to tag // What assets belong to this tag? Yeah i might know the name but releases have the same assets in each (Electrons's SHASUMS256.txt)
endfor

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 24, 2019

Collaborator

No, tag will still include the name (via tag.textContent) and you can use that to find the tag's assets in downloadCounts

Also the selector/code for the tags array needs to be updated to ensure it only selects tags that actually have assets, so we don't trigger a request even when it's not necessary (like on most Releases pages)

Ping on the remaining review comments as well 🙏

dotconnor added some commits May 13, 2019

@bfred-it
Copy link
Collaborator

left a comment

Also these 😅

Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated
Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated
Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated
Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated
wrap(right, <div className="flex-shrink-0">
<span className="mr-2">
{cloudDownload()}
<small className="ml-1">{asset.downloadCount}</small>

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

It may be a good idea to shorten this number

https://github.com/sindresorhus/pretty-bytes

Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated
Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated
Show resolved Hide resolved source/features/show-asset-download-count.tsx Outdated

dotconnor added some commits May 13, 2019

downloadCount
}
}
}`

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

Indentation here can be improved


const tags = select.all('svg.octicon-tag ~ span').map(tag => tag.innerText);
const tagAssets = await getAssetsForTag(tags);
for (const release of select.all('.release-entry:not(.release-timeline-tags)')) {

This comment has been minimized.

Copy link
@bfred-it

bfred-it May 13, 2019

Collaborator

What makes the current implementation “wrong” is that, for example, you’re selecting the tag 3 times in the code (svg.octicon-tag ~ span)

An example solution:

tags = select.all()
downloadCounts = await api(tags)
for tag of tags // reuse `tags` instead of using select.all again
  Add downloadCounts[tag] to tag
endfor
@bfred-it

This comment was marked as resolved.

Copy link
Collaborator

commented May 13, 2019

Also merge with master, it needs a description field because of #2030

dotconnor added some commits May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.