Skip to content

Refresh settings UI and extension branding#34

Open
jonmartin721 wants to merge 15 commits into
mainfrom
settings-onboarding-refresh
Open

Refresh settings UI and extension branding#34
jonmartin721 wants to merge 15 commits into
mainfrom
settings-onboarding-refresh

Conversation

@jonmartin721
Copy link
Copy Markdown
Owner

Summary

  • refresh the settings and onboarding UI with cleaner hidden-state handling and updated GitHub device-flow guidance
  • replace the extension icon and related branding screenshots with the new branch-style mark
  • keep the popup refresh spinner visible long enough to read and restore the toast progress animation used by notifications
  • ignore local .mockups/ previews so design scratch files stay out of git status

Testing

  • npm test -- tests/popup-activity-controller.test.js tests/popup-activity-controller-extra.test.js
  • npm test -- tests/notification-manager.test.js
  • npm run validate

@sentry
Copy link
Copy Markdown

sentry Bot commented Apr 19, 2026

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the UI and standardizes visibility logic by replacing direct style.display manipulations with CSS classes. It introduces a 'settings-surface' design system and enhances the GitHub connection flow with automatic clipboard copying. Feedback identifies an inconsistency where d-none was used instead of the project-standard hidden class in options.js and its tests, and suggests removing an unused transform property from a CSS transition.

Comment thread options/options.js
const itemExpiryEnabled = settings.itemExpiryHours !== null && settings.itemExpiryHours !== undefined;
document.getElementById('itemExpiryEnabled').checked = itemExpiryEnabled;
document.getElementById('itemExpiryInputRow').style.display = itemExpiryEnabled ? 'block' : 'none';
document.getElementById('itemExpiryInputRow').classList.toggle('d-none', !itemExpiryEnabled);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For consistency with the rest of the application's visibility handling (which uses the hidden class), please use hidden instead of d-none. Both classes are defined identically in options.css, but hidden is the more widely used convention in this project.

Suggested change
document.getElementById('itemExpiryInputRow').classList.toggle('d-none', !itemExpiryEnabled);
document.getElementById('itemExpiryInputRow').classList.toggle('hidden', !itemExpiryEnabled);

Comment thread options/options.js
const isEnabled = e.target.checked;
if (isEnabled) {
itemExpiryInputRow.style.display = 'block';
itemExpiryInputRow.classList.remove('d-none');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Use the hidden class instead of d-none to maintain consistency with the project's visibility handling.

Suggested change
itemExpiryInputRow.classList.remove('d-none');
itemExpiryInputRow.classList.remove('hidden');

Comment thread options/options.js
toastManager.info(`Auto-removal enabled: items older than ${hours} hours will be removed`);
} else {
itemExpiryInputRow.style.display = 'none';
itemExpiryInputRow.classList.add('d-none');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Use the hidden class instead of d-none to maintain consistency with the project's visibility handling.

Suggested change
itemExpiryInputRow.classList.add('d-none');
itemExpiryInputRow.classList.add('hidden');

Comment thread options/options.css
border: 1px solid var(--border-color);
border-radius: 18px;
box-shadow: 0 1px 0 rgba(255, 255, 255, 0.03) inset;
transition: border-color 0.2s ease, box-shadow 0.2s ease, background 0.2s ease, transform 0.2s ease;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The transform property is included in the transition list for .settings-surface, but it is not utilized in the hover state or any other state for this base class. Removing it avoids unnecessary transition tracking.

Suggested change
transition: border-color 0.2s ease, box-shadow 0.2s ease, background 0.2s ease, transform 0.2s ease;
transition: border-color 0.2s ease, box-shadow 0.2s ease, background 0.2s ease;

Comment thread tests/options-interactions.test.js Outdated
<input id="itemExpiryEnabled" type="checkbox" />
<input id="itemExpiryHours" value="24" />
<div id="itemExpiryInputRow" style="display:none"></div>
<div id="itemExpiryInputRow" class="d-none"></div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Update the test DOM to use the hidden class instead of d-none to match the suggested changes in the main code.

    <div id="itemExpiryInputRow" class="hidden"></div>

expect(document.getElementById('color-graphite').checked).toBe(true);
expect(document.getElementById('itemExpiryEnabled').checked).toBe(true);
expect(document.getElementById('itemExpiryInputRow').style.display).toBe('block');
expect(document.getElementById('itemExpiryInputRow').classList.contains('d-none')).toBe(false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Update the test expectation to check for the hidden class instead of d-none.

    expect(document.getElementById('itemExpiryInputRow').classList.contains('hidden')).toBe(false);

<input id="itemExpiryEnabled" type="checkbox" />
<input id="itemExpiryHours" />
<div id="itemExpiryInputRow"></div>
<div id="itemExpiryInputRow" class="d-none"></div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Update the test DOM to use the hidden class instead of d-none to match the suggested changes in the main code.

      <div id="itemExpiryInputRow" class="hidden"></div>

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 64cc102afa

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

importModalState.filteredRepos = [...importModalState.repos];

document.getElementById('importLoadingState').style.display = 'none';
document.getElementById('importLoadingState').classList.add('hidden');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Hide import loading pane with a stronger display override

After this change, openImportModal hides #importLoadingState by adding hidden, but in options/options.css the utility rule .hidden { display: none; } is declared before .import-loading { display: flex; }, so the later .import-loading rule wins and the loading pane remains visible even after repos are fetched. In practice, successful imports render the repo list while the spinner block is still shown, which regresses the modal state transition.

Useful? React with 👍 / 👎.

</div>
`;
paginationControls.style.display = 'none';
paginationControls.classList.add('hidden');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use a hide mechanism that can override pagination flex styles

Pagination visibility now depends on toggling the hidden class, but options/options.css defines .pagination-controls { display: flex; } after .hidden { display: none; } with equal specificity. That means classList.add('hidden') does not actually hide the controls, so pagination stays visible even when there are no repositories or only one page.

Useful? React with 👍 / 👎.

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.

1 participant