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

feat: add hide own repository filter checkbox #96

Merged

Conversation

theflucs
Copy link
Contributor

No description provided.

Comment on lines 33 to 37
? repositories?.filter(
// apply hideOwnRepo filter if checked
(repoData) =>
repoData.repository.owner.login !== session?.user.login
)
Copy link
Member

Choose a reason for hiding this comment

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

The logic that filters out own repos is written here and replicated in lines 48-51.

Maybe we should either run it once on top of this function or extract the filtering logic in a separate function, what do you think?

@@ -225,6 +241,16 @@ export default function Stats() {
onChange={(e) => setSearchQuery(e.target.value)}
/>
</div>
<div className="flex sm:items-start items-center">
Copy link
Member

Choose a reason for hiding this comment

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

Here flex-direction is row (by default), you should probably use the justify property.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you seeing them centered?
I see them all left-aligned
Screenshot 2024-01-26 at 09 13 04

Copy link
Member

Choose a reason for hiding this comment

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

They get centered on mobile/smaller screens

Copy link

netlify bot commented Jan 27, 2024

👷 Deploy request for public-github-stats pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 6162cb2

Comment on lines 56 to 58
const filteredOutOwnRepos = filterOutOwnRepos(filteredReposBySearchQuery);

// filter repositories based on search query
const query = searchQuery.toLowerCase();
const filterRepos = repositories?.filter((repoData) =>
repoData.repository.name.toLowerCase().includes(query)
);
return hideOwnRepo ? filteredOutOwnRepos : filteredReposBySearchQuery;
Copy link
Member

Choose a reason for hiding this comment

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

Here you're running filterOutOwnRepos in any case, but if hideOwnRepo is false it is unnecessary.

I'd probably see this better with an if statement to return early, or if we want to keep it more functional with the ternary you should run filterOutOwnRepos directly inside the "then" of the ternary.

Copy link
Member

@Balastrong Balastrong left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@Balastrong Balastrong merged commit 986e9db into DevLeonardoCommunity:main Jan 27, 2024
3 checks passed
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