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: Match the design for the scatterchart #373

Closed
Tracked by #399
bdougie opened this issue Sep 16, 2022 · 12 comments · Fixed by #467
Closed
Tracked by #399

feature: Match the design for the scatterchart #373

bdougie opened this issue Sep 16, 2022 · 12 comments · Fixed by #467

Comments

@bdougie
Copy link
Member

bdougie commented Sep 16, 2022

Current
Screen Shot 2022-09-14 at 3 53 09 PM

Design
Screen Shot 2022-09-14 at 4 07 07 PM

What's missing?

  • Rounded avatars
  • White Border on the avatar
  • Avatars escape the bounds of the chart
  • Today is on the left and needs to be on the right.

@chadstewart can you take a look?

@chadstewart
Copy link
Contributor

I'm not gonna lie, I don't think I can style the images any more that I what I have now other than editing their opacity. Been trying to at least add a border using the eCharts documentation and eCharts is not having it. Been at this for a few hours and just don't understand why it isn't working. I'll keep trying but I very much doubt I'll get any further.

@bdougie bdougie changed the title feat: Match the design from the feat: Match the design for the scatterchart Sep 16, 2022
@bdougie
Copy link
Member Author

bdougie commented Sep 16, 2022

Shall we start looking into the new Nivo chart library? https://nivo.rocks/scatterplot/

Also checking, are you able the other things besides the border?

  • Avatars escape the bounds of the chart
  • Today is on the left and needs to be on the right.

@0-vortex
Copy link
Contributor

echarts bad 🤦

@chadstewart
Copy link
Contributor

chadstewart commented Sep 16, 2022

Shall we start looking into the new Nivo chart library? https://nivo.rocks/scatterplot/

Also checking, are you able the other things besides the border?

  • Avatars escape the bounds of the chart
  • Today is on the left and needs to be on the right.

Was able to add an offset to the avatars but technically I think they can escape from the top of the chart. Currently there isn't data that allows for that. Also am able to move the Today to the other side of the chart.

@bdougie
Copy link
Member Author

bdougie commented Sep 16, 2022

Hey another thought here. What if we add cloudinary processing the images and pass those urls? This would give us the ability transform (border and round) the images, as well as cache them some where.

Alternatively, we could use imagemagick (cli tool for image transforms) + supabase storage to do the same for free.

@brandonroberts have you done anything like this before?

@0-vortex
Copy link
Contributor

Hey another thought here. What if we add cloudinary processing the images and pass those urls? This would give us the ability transform (border and round) the images, as well as cache them some where.

Alternatively, we could use imagemagick (cli tool for image transforms) + supabase storage to do the same for free.

@brandonroberts have you done anything like this before?

I have done this before but i think it is deep overkill / scope creep 🍕

@bdougie bdougie added the high-priority Work on these issues first label Sep 20, 2022
@eryc-cc eryc-cc changed the title feat: Match the design for the scatterchart feature: Match the design for the scatterchart Sep 20, 2022
@brandonroberts
Copy link
Contributor

brandonroberts commented Sep 21, 2022

I've used cropperjs to do something similar to this before for rounding avatars dynamically. https://fengyuanchen.github.io/cropperjs/

https://fengyuanchen.github.io/cropperjs/examples/crop-a-round-image.html

If we run into the same issues with Nivo, or we have to stick with eCharts for now, I'd suggest processing the image first before passing it to eCharts.

@eryc-cc
Copy link
Contributor

eryc-cc commented Sep 21, 2022

I've used cropperjs to do something similar to this before for rounding avatars dynamically. https://fengyuanchen.github.io/cropperjs/

cc @chadstewart

@bdougie
Copy link
Member Author

bdougie commented Sep 22, 2022

... for now, I'd suggest processing the image first before passing it to eCharts.

Took a look at cropperjs and am unsure how to process it outside using the canvas editor. I do agree with processing the image before sending it to the echart is the ideal path.

more research on the scope creep option

I did more research on the cloudinary/imagick suggestion. FWIW, I agree this is scope creep and want to point out that the data on the scatter chart is still not real data. Real data is a higher priority than round avatars.

I found this stackoverflow answer where someone does something similar.

example - https://images.weserv.nl/?url=https://www.github.com/bdougie.png?size=60?v=4&h=300&w=300&fit=cover&mask=circle&maxage=7d

Cloudinary

Here is the above url example using cloudinary https://res.cloudinary.com/demo/image/upload/w_150,h_150,c_fill,g_face,r_max/front_face.png

This would require a cloudinary account.

// uploading js example. If the image exists, it patches in case someone updates their avatar.
new CloudinaryImage("front_face.jpg");

imagemagick

My high level though on imagemagick + a serverless function:

  • check for [user].png in storage
  • leverage imagickmagick to round the avatar
  • upload or update to supabase storage as the [user].png if not present
  • render supabase image urls

s/o on using imagemagick

docs on rounding avatars

@0-vortex
Copy link
Contributor

@bdougie your comment is bugged, infinite scrolling.

Also, I would like to put an end to such discussions, if we are declining to just get the user avatars (90mil) and store them (anywhere), processing those images literally doubles the cost, in storage and processing.

Furthermore, I have mentioned this before, we do have a ready solution for imagemagick that does way more than what you expect, it kind of does everything EXCEPT making round borders.

Please, close this subject forever (yes, high entropy words) or take the code, stop digressing into becoming an image processing company.

Also, we can't put more effort into brainstorming and designing a resize solution than we did in the whole charts library - we are drawing the charts, what do we mean by we can't make it round?!

Delete the charts library, use something else, literally anything that works, don't make the boxes rounded, stop pushing the backend into storing avatars. Do we need to have a full on meeting about why avatars won't happen? It keeps coming up as a subject every now and then?

@bdougie
Copy link
Member Author

bdougie commented Sep 23, 2022

I suppose storing 90 million avatars is indeed expensive. I did not take that into account. I was coming at this from the 150k active Hacktoberfest contributions.

With that said, we can pause this discovery since it is not time well spent. I do believe in this team and know we can eventually solve this or at least redesign.

For now, data integrity is higher priority than rounded avatars.

FWIW, I agree this is scope creep and want to point out that the data on the scatter chart is still not real data. Real data is a higher priority than round avatars.

@bdougie bdougie added blocked and removed high-priority Work on these issues first labels Sep 23, 2022
@bdougie bdougie removed the blocked label Sep 29, 2022
bdougie added a commit that referenced this issue Oct 2, 2022
github-actions bot pushed a commit that referenced this issue Oct 2, 2022
## [1.8.0-beta.10](v1.8.0-beta.9...v1.8.0-beta.10) (2022-10-02)

### 🍕 Features

* Leverage cloudinary for round images ([#467](#467)) ([7dc64d8](7dc64d8)), closes [#373](#373)
@github-actions
Copy link

github-actions bot commented Oct 2, 2022

🎉 This issue has been resolved in version 1.8.0-beta.10 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this issue Oct 3, 2022
## [1.9.0](v1.8.0...v1.9.0) (2022-10-03)

### 🐛 Bug Fixes

* add truncate to contributors repositories list tab ([#472](#472)) ([b7ec96e](b7ec96e)), closes [#460](#460)
* fix merged PRs that looks like closed  ([#475](#475)) ([1dd820a](1dd820a)), closes [#462](#462)
* issue template 👋🏾 ([31b7a7e](31b7a7e))
* moved page head and footer to repo page to avoid reloading the entire screen ([#473](#473)) ([bf790e8](bf790e8)), closes [#449](#449)
* replace churn with overview ([#470](#470)) ([fc0fe3f](fc0fe3f)), closes [#469](#469)
* SEO descriptions ([980e01d](980e01d))

### 🍕 Features

* add filters to hooks for repos and contributions ([#458](#458)) ([05e6bbe](05e6bbe)), closes [#457](#457)
* add page title and info to repo and contributors page ([#459](#459)) ([c0000e4](c0000e4)), closes [#446](#446)
* add pagination to contributors page ([#454](#454)) ([37f4da8](37f4da8)), closes [#403](#403)
* Add swr cache ([#437](#437)) ([6b2b1cb](6b2b1cb)), closes [#308](#308)
* connect scatter chart to contributors data ([#466](#466)) ([be78024](be78024)), closes [#465](#465)
* hide show/hide buttons temporarily ([#474](#474)) ([2ce5b3c](2ce5b3c)), closes [#439](#439)
* Leverage cloudinary for round images ([#467](#467)) ([7dc64d8](7dc64d8)), closes [#373](#373)
* update filters for repos and contributors ([#464](#464)) ([e15bbf8](e15bbf8)), closes [#461](#461)
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this issue Aug 17, 2023
## [1.8.0-beta.10](open-sauced/app@v1.8.0-beta.9...v1.8.0-beta.10) (2022-10-02)

### 🍕 Features

* Leverage cloudinary for round images ([#467](open-sauced/app#467)) ([7dc64d8](open-sauced/app@7dc64d8)), closes [#373](open-sauced/app#373)
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this issue Aug 17, 2023
## [1.9.0](open-sauced/app@v1.8.0...v1.9.0) (2022-10-03)

### 🐛 Bug Fixes

* add truncate to contributors repositories list tab ([#472](open-sauced/app#472)) ([b7ec96e](open-sauced/app@b7ec96e)), closes [#460](open-sauced/app#460)
* fix merged PRs that looks like closed  ([#475](open-sauced/app#475)) ([1dd820a](open-sauced/app@1dd820a)), closes [#462](open-sauced/app#462)
* issue template 👋🏾 ([31b7a7e](open-sauced/app@31b7a7e))
* moved page head and footer to repo page to avoid reloading the entire screen ([#473](open-sauced/app#473)) ([bf790e8](open-sauced/app@bf790e8)), closes [#449](open-sauced/app#449)
* replace churn with overview ([#470](open-sauced/app#470)) ([fc0fe3f](open-sauced/app@fc0fe3f)), closes [#469](open-sauced/app#469)
* SEO descriptions ([980e01d](open-sauced/app@980e01d))

### 🍕 Features

* add filters to hooks for repos and contributions ([#458](open-sauced/app#458)) ([05e6bbe](open-sauced/app@05e6bbe)), closes [#457](open-sauced/app#457)
* add page title and info to repo and contributors page ([#459](open-sauced/app#459)) ([c0000e4](open-sauced/app@c0000e4)), closes [#446](open-sauced/app#446)
* add pagination to contributors page ([#454](open-sauced/app#454)) ([37f4da8](open-sauced/app@37f4da8)), closes [#403](open-sauced/app#403)
* Add swr cache ([#437](open-sauced/app#437)) ([6b2b1cb](open-sauced/app@6b2b1cb)), closes [#308](open-sauced/app#308)
* connect scatter chart to contributors data ([#466](open-sauced/app#466)) ([be78024](open-sauced/app@be78024)), closes [#465](open-sauced/app#465)
* hide show/hide buttons temporarily ([#474](open-sauced/app#474)) ([2ce5b3c](open-sauced/app@2ce5b3c)), closes [#439](open-sauced/app#439)
* Leverage cloudinary for round images ([#467](open-sauced/app#467)) ([7dc64d8](open-sauced/app@7dc64d8)), closes [#373](open-sauced/app#373)
* update filters for repos and contributors ([#464](open-sauced/app#464)) ([e15bbf8](open-sauced/app@e15bbf8)), closes [#461](open-sauced/app#461)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants