Skip to content

Conversation

webfiltered
Copy link
Contributor

@webfiltered webfiltered commented Sep 13, 2025

Summary

Adds a basic dialog endpoint for the desktop app.

Changes

  • Adds a route for the desktop app to load individual windows: /desktop-dialog
  • Uses GET vars to request the dialog contents

Caveats

  • Uses existing hard-coded strings; i18n can be added separately
  • Asserts type of inbound buttons array; schema validation can be added if necessary, however I would argue that should wait until this is separated out from the primary Vue app
  • Requires rebase Add Inter font used by Figma designs #5535

Screenshots (if applicable)

image

┆Issue is synchronized with this Notion page by Unito

@webfiltered webfiltered requested a review from a team as a code owner September 13, 2025 22:17
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 13, 2025
Copy link

github-actions bot commented Sep 13, 2025

🎭 Playwright Test Results

🕵🏻 No test results found

⏰ Completed at: 09/15/2025, 10:02:56 AM UTC

📊 Test Reports by Browser

  • chromium: Deployment failed
  • chromium-2x: Deployment failed
  • chromium-0.5x: Deployment failed
  • mobile-chrome: Deployment failed

🎉 Click on the links above to view detailed test results for each browser configuration.

Copy link
Contributor

@DrJKL DrJKL left a comment

Choose a reason for hiding this comment

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

Mostly CSS comments 😸

@reference '../assets/css/style.css';

.dialog-title {
font-family: 'ABC ROM';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you register the font in style.css and use the tailwind utility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably be draft, sorry~

The font isn't in the repo yet; and this PR shouldn't add it. But yes, it annoyed me to have to create a class just for this. 😃

}

.p-button {
@apply rounded-lg;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please just add the class to the button, I'm trying to get rid of all of the @applys and prohibit them with a linter soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the others <3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry you had to read that. The scoped styles are ~80% generated slop - which I pray does not come as a surprise.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤖

.p-button-secondary {
@apply text-white rounded-lg border-none;

background: var(--color-button-background, rgba(255, 255, 255, 0.15));
Copy link
Contributor

Choose a reason for hiding this comment

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

bg-button-background

}

.p-button-secondary:hover {
background: rgba(255, 255, 255, 0.25);
Copy link
Contributor

Choose a reason for hiding this comment

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

bg-white/25

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar for the ones below

}

.p-button-danger {
background: rgba(241, 67, 82, 0.5);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you name the base color and put it into the color palette?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for the others

Comment on lines +110 to +111
color: #f0ff41;
border: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should inherit from the original declaration, no need to add them for each modifier state if you're not changing the value.

@webfiltered webfiltered marked this pull request as draft September 13, 2025 22:46
@webfiltered
Copy link
Contributor Author

Replaced by #5605.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants