-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
chore: partners/sponsors page #7991
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7991 +/- ##
=======================================
Coverage 76.60% 76.61%
=======================================
Files 115 115
Lines 9602 9606 +4
Branches 322 322
=======================================
+ Hits 7356 7360 +4
Misses 2245 2245
Partials 1 1 ☔ View full report in Codecov by Sentry. |
1efa109
to
81522ce
Compare
269d6aa
to
7e19b83
Compare
7e19b83
to
8724b52
Compare
8724b52
to
60f5bfa
Compare
60f5bfa
to
5960304
Compare
I’m reaching out to Kylie on Slack to get the logos of the missing companies, since I couldn’t find the brand kits for some of them or they require permission. |
5960304
to
545c769
Compare
545c769
to
77f675c
Compare
d22e42a
to
d45ec56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing *
imports, all of these are default exports should just be:
import { default as ARM } from './ARM';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, because these are folders and have Favicon and Logo files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bjohansebas can you please inspect each one of the SVG files you added and ensure they all have the same headers, reasonable viewports and no accidental comments/white spaces or properties that aren't being used? (Anything that is not a path
element)
<polygon | ||
id="Shape_30_" | ||
fill="currentColor" | ||
points="759.311,347.761 796.157,347.761 806.062,319.83 749.505,319.83 " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: lots of white spaces here?
id="Livello_1" | ||
xmlns="http://www.w3.org/2000/svg" | ||
xmlnsXlink="http://www.w3.org/1999/xlink" | ||
x="0px" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these shouldn't be here IMO (check my comment about you double checking all the SVGs)
d="M49.716 57.7644C31.2403 52.2992 33.3293 99.8793 54.868 86.8433C61.0841 83.0806 69.2772 77.6726 79.0841 77.6726C87.9313 77.6726 92.772 87.2578 101.619 87.2578C114.363 85.8708 113.815 61.6781 105.206 60.8951C96.1535 60.8951 88.7351 76.1836 77.0194 72.5908C66.904 69.4893 56.5224 59.7773 49.716 57.7644ZM52.2164 50.0945C61.6535 52.886 69.874 61.9169 79.4022 64.8381C80.1779 65.076 80.8375 65.0818 81.6183 64.8701C83.4421 64.3762 85.5311 62.8377 87.0285 61.7043C92.5559 57.5203 97.6388 52.8687 105.002 52.8687H105.371L105.738 52.9024C124.711 54.6274 124.528 93.4875 102.32 95.9048L101.882 95.9526H101.334C100.632 95.9526 99.9279 95.9318 99.2316 95.8455C93.5185 95.1364 89.6214 92.5508 85.5659 89.7106C83.9513 88.5799 81.1622 86.4363 79.0675 86.4363C71.8124 86.4363 65.2729 90.7137 59.2521 94.3582C53.3263 97.9447 46.4174 99.2437 40.1305 95.7347C32.7121 91.5942 29.5177 82.5128 29.1909 74.3252C28.8514 65.828 31.5307 55.8467 39.2685 51.3953C43.2868 49.0838 47.8298 48.7968 52.2164 50.0945Z" | ||
fill="url(#paint0_linear_1656_9)" | ||
/> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the only util for partners, this shouldn't be its own folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that these JSON files, including the download ones should be moved to the public folder IMO. /apps/site/static/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @avivkeller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? They don't need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't, but I feel weird having static data files deep within a source folder of utilities.
Either that or on the root of the apps/site together with the remaining json files.
async function fetchOpenCollectiveData() { | ||
const endpoint = 'https://api.opencollective.com/graphql/v2'; | ||
|
||
const query = `{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use their REST API please? https://docs.opencollective.com/help/contributing/development/api I'd like to remove the need of adding yet another layer of complexity (GraphQL) here
@@ -0,0 +1,95 @@ | |||
// This is used to ensure that URLs are always in the correct format | |||
function fixUrl(url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem like this should be here, like in this file. Also, why we need to replace these to https? The change to https should be done automatically by the website/url if they support https, otherwise we're forcefully switching to https schema when a website could maybe not even support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply let the browser handle non-https links, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump, @bjohansebas
apps/site/layouts/Post.tsx
Outdated
{children} | ||
|
||
<WithBlogCrossLinks /> | ||
{/* {type === 'vulnerability' && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @bjohansebas any reason this is commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there isn’t any security partner we can include — none of them have an agreement with the foundation. That’s why I commented it out for now, to see if anything happened during all this time, but it seems like nothing has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simply remove it instead of commenting it out and add this as a comment on our main issue?
@@ -0,0 +1,92 @@ | |||
import type { PartnerCategory, Partners } from '#site/types/partners.js'; | |||
|
|||
function randomPartnerList( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function feels too complex:
- You can achieve similar Mullbery results with Node's built-in crypto package
- Instead of creating a Date with fixed components, use Date.now() and use Math.abs() / dividing number by the exponent that would be the hour component.
- No need to reshuffle them on every call,
- From the beginning the set of partners should be unique, doing the whole filter, slice, weightening, shuffle etc... Too many operations.
- Let's remove custom sort functions and what not. This function is doing way too many things IMO. This function should have not more than 10~ lines, and I don't think we need anything else or more than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @avivkeller any suggestion here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @bjohansebas any update here? (cc @avivkeller)
apps/site/components/Common/Partners/PartnersLogoList/index.tsx
Outdated
Show resolved
Hide resolved
apps/site/components/Common/Partners/PartnersLogoList/index.tsx
Outdated
Show resolved
Hide resolved
apps/site/components/Common/Partners/PartnersIconList/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bjohansebas I finally finished reviewing this. I do believe this PR needs a rebase and some cleanup.
- Please verify all SVGs
<svg>
properties (ensure you have a commonset)- Fix the formatting and remove unnecessary nodes/elements
- Address the PR comments (cleanup, organization, code complexity, etc)
We're almost there :) -- Also, @kyliewd is the content of the PR in a ready state? Have you reviewed the preview? Does it look good? Do we copywriting?
const initialRenderer = useRef(true); | ||
|
||
const [seedList, setSeedList] = useState<Array<Partners>>(() => { | ||
if (maxLength === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do a logos.slice(0, maxLength || logos.length)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this? 🤔
const [seedList, setSeedList] = useState<Array<Partners>>(() => {
const filteredLogos = logos.filter(
partner => !categories || partner.categories.includes(categories)
);
return filteredLogos.slice(0, maxLength || filteredLogos.length);
});
Description
closes #7909
I’m making changes quite quickly, so the code is not ready for review yet.
Validation
Related Issues
Check List
pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.