feat: add clickable status badges for GitHub Projects status changes#78
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @kubrickcode, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for managing GitHub Projects by introducing interactive status badges directly within issue lists. Users can now click on a project status badge to reveal a dropdown menu, enabling them to update an issue's status without navigating away from the current page. This feature is powered by integrating GitHub's GraphQL API for write operations, supported by an updated OAuth scope, and includes an optimistic UI for a seamless and responsive interaction. The project's build process has also been streamlined with the adoption of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
github-actions
Bot
added
improvement
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent new feature: clickable status badges that allow users to change an issue's project status directly from the list view. The implementation is robust, featuring an optimistic UI with rollback on failure, good separation of concerns with new services for the dropdown and badge rendering, and strong adherence to accessibility best practices. My review focuses on a few areas to further improve code quality and align with the repository's style guide, such as reducing minor code duplication, consistently applying function argument patterns, and extracting constants. Overall, this is a well-executed and valuable addition.
| .project-status-badge--clickable.project-status-badge--compact:hover { | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
This CSS rule is redundant. The .project-status-badge--clickable class, defined on line 141, already sets cursor: pointer;. This rule can be safely removed to avoid unnecessary code.
| const addStatusBadge = async ( | ||
| issueNumber: number, | ||
| status: string, | ||
| color: string | null, | ||
| projectId: string | null, | ||
| projectItemId: string | null, | ||
| statusFieldId: string | null, | ||
| statusOptions: StatusOption[] | null | ||
| ) => { |
There was a problem hiding this comment.
This function addStatusBadge takes 7 arguments, which can make it hard to read and prone to errors. According to the style guide, functions with two or more arguments should use an object for parameters to improve readability and make future modifications easier.1
I suggest refactoring this to accept a single object parameter.
Footnotes
-
Style Guide (lines 110-116): "Object form in most situations with 2+ arguments." ↩
| badge.addEventListener("click", (e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onBadgeClick({ | ||
| badge, | ||
| currentStatus: status, | ||
| issueNumber, | ||
| projectId, | ||
| projectItemId, | ||
| statusFieldId, | ||
| statusOptions, | ||
| }); | ||
| }); | ||
|
|
||
| badge.addEventListener("keydown", (e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onBadgeClick({ | ||
| badge, | ||
| currentStatus: status, | ||
| issueNumber, | ||
| projectId, | ||
| projectItemId, | ||
| statusFieldId, | ||
| statusOptions, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The logic for handling the click and keydown events is duplicated. To adhere to the "Don't Repeat Yourself" (DRY) principle and the style guide's rule on modularization, you can extract the shared logic into a separate handler function.1
const handleInteraction = (e: MouseEvent | KeyboardEvent) => {
e.preventDefault();
e.stopPropagation();
onBadgeClick({
badge,
currentStatus: status,
issueNumber,
projectId,
projectItemId,
statusFieldId,
statusOptions,
});
};
badge.addEventListener("click", handleInteraction);
badge.addEventListener("keydown", (e) => {
if (e.key === "Enter" || e.key === " ") {
handleInteraction(e);
}
});Footnotes
-
Style Guide (lines 34-37): "While it might be missed when contexts are far apart, code repetition is absolutely prohibited in recognized situations. Modularize similar pattern repetitions (not just identical code) into reusable forms." ↩
| export const updateBadgeStatus = ( | ||
| issueNumber: number, | ||
| newStatus: string, | ||
| newColor: string | null | ||
| ): void => { | ||
| const badge = document.querySelector<HTMLElement>( | ||
| `.${CSS_CLASSES.BADGE}[data-issue-number="${issueNumber}"]` | ||
| ); | ||
|
|
||
| if (!badge) return; | ||
|
|
||
| badge.setAttribute("data-status", newStatus); | ||
| badge.style.setProperty("--status-color", newColor || UI_DEFAULTS.BADGE_COLOR); | ||
|
|
||
| const isCompact = badge.classList.contains(CSS_CLASSES.BADGE_COMPACT); | ||
| if (isCompact) { | ||
| badge.title = newStatus; | ||
| badge.setAttribute("aria-label", `Status: ${newStatus}`); | ||
| } else { | ||
| badge.textContent = newStatus; | ||
| } | ||
| }; |
There was a problem hiding this comment.
This function updateBadgeStatus takes 3 arguments. According to the style guide, functions with 2 or more arguments should generally use an object for parameters to improve readability and make it easier to add/remove arguments in the future.1
I suggest refactoring this to accept a single object parameter. This will also make the call sites in content.ts more explicit.
export const updateBadgeStatus = ({
issueNumber,
newStatus,
newColor,
}: {
issueNumber: number;
newStatus: string;
newColor: string | null;
}): void => {
const badge = document.querySelector<HTMLElement>(
`.${CSS_CLASSES.BADGE}[data-issue-number="${issueNumber}"]`
);
if (!badge) return;
badge.setAttribute("data-status", newStatus);
badge.style.setProperty("--status-color", newColor || UI_DEFAULTS.BADGE_COLOR);
const isCompact = badge.classList.contains(CSS_CLASSES.BADGE_COMPACT);
if (isCompact) {
badge.title = newStatus;
badge.setAttribute("aria-label", `Status: ${newStatus}`);
} else {
badge.textContent = newStatus;
}
};Footnotes
-
Style Guide (lines 110-116): "Object form in most situations with 2+ arguments." ↩
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent new feature that allows users to change a project's status directly from the issue list. The implementation is robust, featuring a clean separation of concerns with the new status-dropdown.service.ts, optimistic UI updates with error rollbacks, and the use of esbuild for a streamlined build process. My review focuses on a few areas to enhance code quality and ensure alignment with the repository's coding conventions, primarily by reducing code duplication and adhering to function signature guidelines. Overall, this is a very strong contribution.
Implement dropdown UI to change status directly from issue list like Linear - Integrate GitHub GraphQL updateProjectV2ItemFieldValue mutation - Change OAuth scope from read:project to project (write access) - Optimistic UI for instant feedback with rollback on failure - Add esbuild bundler for browser compatibility - Add message validation and input validation fix #40
kubrickcode
force-pushed
the
develop/shlee/40
branch
from
875a9bd to
15af75c
Compare
|
/gemini review |
1 participant
Implement dropdown UI to change status directly from issue list like Linear
fix #40