Skip to content

Add tests on litegraph widget text truncate #191

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
Merged
merged 4 commits into from
Jul 21, 2024
Merged

Add tests on litegraph widget text truncate #191

merged 4 commits into from
Jul 21, 2024

Conversation

christian-byrne
Copy link
Contributor

huchenlei
huchenlei approved these changes Jul 21, 2024
View reviewed changes
Copy link
Contributor

@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! Thanks for your hard work!

@huchenlei
Copy link
Contributor

BTW, we have a github action to automatically update linux-based screenshots, so you don't need to do the freshcheckout and have the default model selected to set expectation. To trigger that you just need to add label New Browser Test Expectations.

@huchenlei
Copy link
Contributor

Small rendering diff. Setting expectation with Github action
image

@huchenlei huchenlei added the New Browser Test Expectations New browser test screenshot should be set by github action label Jul 21, 2024
@christian-byrne
Copy link
Contributor Author

BTW, we have a github action to automatically update linux-based screenshots, so you don't need to do the freshcheckout and have the default model selected to set expectation. To trigger that you just need to add label New Browser Test Expectations.

Oh that's good. Should I remove the screenshots from the PR and re -run to set them?

@christian-byrne
Copy link
Contributor Author

Small rendering diff. Setting expectation with Github action image

Is one of those using Roboto Mono and the other not?

@huchenlei
Copy link
Contributor

Small rendering diff. Setting expectation with Github action image

Is one of those using Roboto Mono and the other not?

Not sure. I find this font issue only solvable if github action set the expectation by itself. 🤣

@huchenlei huchenlei added New Browser Test Expectations New browser test screenshot should be set by github action and removed New Browser Test Expectations New browser test screenshot should be set by github action labels Jul 21, 2024
@christian-byrne
Copy link
Contributor Author

Small rendering diff. Setting expectation with Github action image

Is one of those using Roboto Mono and the other not?

Not sure. I find this font issue only solvable if github action set the expectation by itself. 🤣

Lol, I understand how it's supposed to go now

@huchenlei
Copy link
Contributor

Hmmm, it might lack the permission to checkout your branch?

@christian-byrne
Copy link
Contributor Author

Hmmm, it might lack the permission to checkout your branch?

Not sure. Seems to work now though

@huchenlei
Copy link
Contributor

huchenlei commented Jul 21, 2024 •
edited
Loading

Let me get it merged first and I am going to make another PR to update the test expectation with GA. I am going to merge it to the next release, v1.2 branch to avoid myself some trouble of merging back from main.

@huchenlei huchenlei changed the base branch from main to dev1.2 July 21, 2024 23:51
@huchenlei huchenlei merged commit 1110729 into Comfy-Org:dev1.2 Jul 21, 2024
2 of 3 checks passed
huchenlei added a commit that referenced this pull request Jul 22, 2024
@huchenlei @christian-byrne
v1.2.0 Side Bar & Menu rework (#189)
65740a3 
* Basic side tool bar skeleton + Theme toggle (#164)

* Side bar skeleton

* Fix grid layout

* nit

* Add theme toggle logic

* Change primevue color theme to blue to match beta menu UI

* Add litegraph canvas splitter overlay (#177)

* Add vue wrapper

* Splitter overlay

* Move teleport to side bar comp

* Toolbar placeholder

* Move settings button from top menu to side bar (#178)

* Reverse relationship between splitter overlay and sidebar component (#180)

* Reverse relationship between splitter overlay and sidebar component

* nit

* Remove border on splitter

* Fix canvas shift (#186)

* Move queue/history display to side bar (#185)

* Side bar placeholder

* Pinia store for queue items

* Flatten task item

* Fix schema

* computed

* Switch running / pending order

* Use class-transformer

* nit

* Show display status

* Add tag severity style

* Add execution time

* nit

* Rename to execution success

* Add time display

* Sort queue desc order

* nit

* Add remove item feature

* Load workflow

* Add confirmation popup

* Add empty table placeholder

* Remove beta menu UI's queue button/list

* Add tests on litegraph widget text truncate (#191)

* Add tests on litegraph widget text truncate

* Updated screenshots

* Revert port change

* Remove screenshots

* Update test expectations [skip ci]

* Add back menu.settingsGroup for compatibility (#192)

* Close side bar on menu location set as disabled (#194)

* Remove placeholder side bar tabs (#196)

---------

Co-authored-by: bymyself <abolkonsky.rem@gmail.com>
Co-authored-by: github-actions <github-actions@github.com>
@christian-byrne christian-byrne deleted the widget-text-truncate-tests branch July 24, 2024 00:20
@Steudio
Copy link

Steudio commented Aug 22, 2024

Shouldn't the widget name be truncated instead of the widget value?
From a user perspective, the widget value is the most important and it is currently impossible to see the widget value without resizing.
It should look like the image on the right:
widget

@christian-byrne
Copy link
Contributor Author

Shouldn't the widget name be truncated instead of the widget value? From a user perspective, the widget value is the most important and it is currently impossible to see the widget value without resizing. It should look like the image on the right: widget

Generally I agree with you.

When I tried that, there were situations in which the input value was very long (checkpoints, file paths, text strings), and then the parameter name was essentially always hidden. The only way to see the parameter name in those situations was to expand the node extremely wide (sometimes off screen). Conversely, to see the full input value you can just click the widget.

@Steudio
Copy link

Steudio commented Aug 22, 2024

I understand your point and I would still argue that having a direct visual access to the widget value is more important than the widget name (when choosing to have the smallest node footprint).

  1. Some long input value will never be compatible with small footprint workflow, so be it.
  2. I feel that some widget values are self-explanatory and the widget name become less important (at least to advanced user?).
  3. A mouse over popup showing the covered name should be enough to provide the hidden name (I did notice that some nodes already have a description of the widget, which is great)

mouse over example:
image

Regarding clicking the widget to show the whole value name, will it be possible to always show what the current value is?
I notice that some widget does show the currently selected value, but some do not.
image

@christian-byrne
Copy link
Contributor Author

To me, all your points seem valid. I will have to try to get some other opinions.

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
New Browser Test Expectations New browser test screenshot should be set by github action
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@christian-byrne @huchenlei @Steudio