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.

Sign up for GitHub

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

Jump to bottom

Front stack primary updates and improvements #757

Merged
merged 7 commits into from
Sep 10, 2024
Merged

Front stack primary updates and improvements #757

merged 7 commits into from
Sep 10, 2024

Conversation

dmx974
Copy link
Contributor

@dmx974 dmx974 commented Sep 8, 2024 •
edited
Loading

Mandatory changes to prepare the front stack for the upcoming updates and improvements (i.e. tabs, routing, autoimport icons, tailwind ...)

@dmx974
(fix) index.html formating for prettier
8a048c0
@dmx974
(add) proper icon management
28d7a97 
- on-demand icons auto importing
- handle all available icon sets (https://icones.js.org)
@socket-security Socket Security
Copy link

socket-security bot commented Sep 8, 2024 •
edited
Loading

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@iconify/json@2.2.245 filesystem +2 353 MB cyberalien
npm/unplugin-icons@0.19.3 Transitive: environment, filesystem, shell +18 2.27 MB antfu, hannoeru, sxzz, ...1 more
npm/unplugin-vue-components@0.27.4 environment, filesystem +43 3.44 MB antfu
npm/vue-router@4.4.3 environment +1 859 kB posva

View full report↗︎

@dmx974
(fix) proper css management
b995940
@dmx974
(add) front stack improvement:
7560de7 
- implement vue router
- prepare for App.vue simplification
- proper management of views and layouts
- fix Tailwind CSS and prepare for overall css cleaning
@dmx974 dmx974 changed the title (PR) proper icon management Front stack primary updates and improvements Sep 8, 2024
@huchenlei
Copy link
Member

Mandatory changes to prepare the front stack for the upcoming updates and improvements (i.e. tabs, routing, autoimport icons, tailwind ...)

The failing playwright test is caused by tailwind altering the appearance of the legacy menu. See following picture for reference.

Idealy we don't want to affect people who use the legacy menu.
image

huchenlei
huchenlei requested changes Sep 9, 2024
View reviewed changes
Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

I would like to see the PR breaking down into a few smaller ones:

  • Format index.html / vite.config.mts
  • Add router
  • Add iconify
  • Fix tailwindcss

src/main.ts Outdated
import { i18n } from './i18n'
import '@comfyorg/litegraph/style.css'
import '@/assets/css/style.css'
import '@/assets/css/user.css'
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the change to user.css as it is for users to modify in the final build target to add extra rules. Importing user.css here would cause user.css not appear in the final build target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy that!

@huchenlei
Copy link
Member

TailwindCSS setup is done in #768. You can now safely proceed with rest of the PRs with tailwind classes.

huchenlei
huchenlei approved these changes Sep 10, 2024
View reviewed changes
Copy link
Member

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

LGTM

@huchenlei huchenlei merged commit 05b3ad2 into main Sep 10, 2024
6 checks passed
@huchenlei huchenlei deleted the ux-improvement branch September 10, 2024 23:53
@huchenlei huchenlei mentioned this pull request Sep 22, 2024
Merged
@mcmonkey4eva
Copy link
Contributor

This PR hardcoded the frontend to only work as a webroot, ie owner of the / route, which is not necessarily true for all deployments, and in fact actively is not in many cases. SwarmUI for example does not deploy comfy at the webroot, and there are many users with special apache2/nginx configured subroutes and similar.

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

@huchenlei huchenlei huchenlei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmx974 @huchenlei @mcmonkey4eva