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.

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

feat(ui): Add image size badge to gallery images #5632

Merged
merged 7 commits into from Mar 12, 2024

Conversation

rohinish404
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because:

Have you updated all relevant documentation?

  • Yes
  • No

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Merge Plan

Added/updated tests?

  • Yes
  • No : please replace this line with details on why tests
    have not been included

[optional] Are there any post deployment tasks we need to perform?

@github-actions github-actions bot added the frontend PRs that change frontend files label Feb 1, 2024
@rohinish404 rohinish404 marked this pull request as ready for review February 1, 2024 18:25
@hipsterusername
Copy link
Member

@rohinish404 Can you share how this works? If it's always displayed, it might end up as clutter. If it shows on hover, that could be good information.

@rohinish404
Copy link
Contributor Author

@rohinish404 Can you share how this works? If it's always displayed, it might end up as clutter. If it shows on hover, that could be good information.

yeah @psychedelicious provided some suggestions on the visibility of this in the original issue. I didn't get that so made it always visible for now. Will work on that next. Should it be on hover?

@psychedelicious
Copy link
Collaborator

psychedelicious commented Feb 1, 2024

@rohinish404 Can you please add a screenshot or short video demonstrating the feature? Also test out changing the gallery image size to see how that interacts.

@rohinish404
Copy link
Contributor Author

@rohinish404 Can you please add a screenshot or short video demonstrating the feature? Also test out changing the gallery image size to see how that interacts.

Screenshot 2024-02-02 at 9 53 13 AM
Screen.Recording.2024-02-02.at.9.55.14.AM.mov

@joshistoast
Copy link
Contributor

My feedback would be to add a setting for displaying image resolutions in the gallery and have it disabled by default

@hipsterusername
Copy link
Member

Aye - I think showing on all images all the time may not be desirable. Should consider an on hover.

additionally, we should look at styling this to be more in line with current theme/components

@rohinish404
Copy link
Contributor Author

Aye - I think showing on all images all the time may not be desirable. Should consider an on hover.

additionally, we should look at styling this to be more in line with current theme/components

I was first tying to implement for smaller image size this badge wont be displayed as suggested by @psychedelicious . If we want to make this thing on the basis of hover then there's no need to implement this conditional feature right?

@psychedelicious
Copy link
Collaborator

I was first tying to implement for smaller image size this badge wont be displayed as suggested by @psychedelicious . If we want to make this thing on the basis of hover then there's no need to implement this conditional feature right?

Making the badge overlay optional would be good. It could be on hover by default, with an option to enable it always.

Either way, the badge will need to hide itself if the images are too small.

@rohinish404 rohinish404 marked this pull request as draft February 12, 2024 03:51
@rohinish404
Copy link
Contributor Author

making the pr draft for now..im a bit busy with exams, will continue after that :)

@rohinish404 rohinish404 marked this pull request as ready for review February 20, 2024 07:30
@rohinish404
Copy link
Contributor Author

@psychedelicious for image size less than 80px, the image badge wont be displayed now :)

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Nice, that works great. 80px is a good size.

Next step is to add some styling. Could try like a 0.7 opacity bg with base.50 text. Maybe give the top-left corner a borderRadius base. Just some ideas for styling.

Then we probably want this to be configurable. I think there was some feedback already on this from other users

@rohinish404
Copy link
Contributor Author

rohinish404 commented Feb 20, 2024

Nice, that works great. 80px is a good size.

Next step is to add some styling. Could try like a 0.7 opacity bg with base.50 text. Maybe give the top-left corner a borderRadius base. Just some ideas for styling.

Then we probably want this to be configurable. I think there was some feedback already on this from other users

should i go with display on hover? It was looking good

@rohinish404
Copy link
Contributor Author

rohinish404 commented Feb 20, 2024

@psychedelicious fixed the requested things, did a bit of styling (need feedback on this), and made the badge show on hover. @hipsterusername can you test the hover functionality?

@rohinish404
Copy link
Contributor Author

Screen.Recording.2024-02-23.at.12.39.48.AM.mov

@rohinish404
Copy link
Contributor Author

i just noticed it..the del icon is on top of badge need to fix that!

@hipsterusername
Copy link
Member

Lets try to style it in such a way that it's clean and not overlapping things - Think white on black fits theme more.

image

@rohinish404
Copy link
Contributor Author

Lets try to style it in such a way that it's clean and not overlapping things - Think white on black fits theme more.

image

I made some changes to avoid overlapping. Also reduced the font size because for smaller image size the badge was going out of the image :)
https://github.com/invoke-ai/InvokeAI/assets/92542124/4aad19ff-a152-456a-a1fb-ab13ce42212f

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Reminder this needs to be configurable

@rohinish404
Copy link
Contributor Author

Reminder this needs to be configurable

Already on hover..anything else?

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Thanks! I made some minor changes to the styling and added a setting to always show the dimensions:

image

@psychedelicious psychedelicious enabled auto-merge (rebase) March 12, 2024 07:52
@psychedelicious psychedelicious merged commit 43948e0 into invoke-ai:main Mar 12, 2024
14 checks passed
@rohinish404
Copy link
Contributor Author

Thanks! I made some minor changes to the styling and added a setting to always show the dimensions:

image

Thanks for the merge!! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[enhancement]: image gallery upscaling improvements
4 participants