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

Update scene, image, and gallery detail panels #4582

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

QxxxGit
Copy link

@QxxxGit QxxxGit commented Feb 17, 2024

  • Updates the display of metadata on the detail panels
  • Removes studio logo from detail panels
  • Uses Show all details setting to persist expanded details/description

@WithoutPants
Copy link
Collaborator

Thanks for working on this. In future, I think changes like this would benefit from showing some screenshots so people can provide feedback without having to build the branch.

I think the handling of empty fields needs improving:
image

I think empty fields shouldn't be shown at all. For rating specifically, having the empty rating control displayed infers to the user that it can be interacted with.

I personally think losing the studio image from the detail pages is a step backwards.

The expand details button probably needs a rethink as well.

image

The button area is very large - it would probably benefit from being a minimal variant. As shown above, it's also shown when it doesn't need to be. A Show more.../Show less... button at the end of the details text, shown only when necessary is probably the better way of doing this imo. When expanding the details, I also noticed that we get some movement in the elements from the scrollbar popping in and out.

Performer cards are now aligned to the left instead of being centered, which I think looks wonky on smaller viewports.

@DogmaDragon
Copy link
Collaborator

I personally think losing the studio image from the detail pages is a step backwards.

Can we reuse or create a new toggle for show studios as text?
image

The image takes up a lot of space while being proportionally no more important than other data fields.

@echo6ix
Copy link
Contributor

echo6ix commented Feb 22, 2024

I've briefly tested this build. I suspect much of my feedback may be regarding incomplete work, but I thought I'd write this out anyway.

1. CSS and Layout

The layout displays detail items titles (aka the item heading) and detail item values using h5 and h6 tags respectively. This contributes to an inconsistent look with the other detail pages (performer, tag, studios) that we are trying to emulate.

Why not just re-use the CSS and the structure those pages are using:

    <div class="detail-item {NAME}">
        <span class="detail-item-title {NAME}">...</span>
        <span class="detail-item-value {NAME}">...</span>
    </div>

Where {NAME} is a placeholder for the title field title of each detail-item. For example,

<div class="detail-group">
    <div class="detail-item studio">
        <span class="detail-item-title studio">...</span>
        <span class="detail-item-value studio">...</span>
    </div>
    <div class="detail-item release-date">
        <span class="detail-item-title release-date">...</span>
        <span class="detail-item-value release-date">...</span>
    </div>
    <div class="detail-item studio-code">
        <span class="detail-item-title studio-code">...</span>
        <span class="detail-item-value studio-code">...</span>
    </div>
</div>

Aside from the consistency, reliability, and prospective ease of maintenance by reusing the same CSS and structure, I suspect this will make it a lot easier to add the order property to above CSS. The order property is important because it allows the devs (and users through custom css) to easily change the order of detail items.

detail-item.studio { order: 1; }
detail-item.release-date  { order: 2; }
detail-item.studio-code { order: 3; }
detail-item.tags { order: 4; }
detail-item.updated-date  { order: 5; }
detail-item.created-date  { order: 6; }
detail-item.details { order: 7; }
detail-item.director { order: 8; }
...

2. What does the date heading mean?

I believe we should take this opportunity to rename the date heading and its corresponding class name to Release date and release-date respectively. While this item certainly is a date, I think we should specify what the date actually refers to, a [scene] release date. Some people may use it for other purposes, but by and large, all the scrapers are populating a release date.

3. Detail items with null values should not be displayed

When detail items have null values they're still being displayed. Per consistency with the detail pages we're imitating, it should not do this.

4. Created at and Updated at dates need styling

Those values still don't have the same styling as the other detail items

5. Details expand/collapse button

I'm thinking it might be optimal to replace this button with show more/show less functionality appended at the end of the details text. See example. Why? When the user reads the details, and when the details are especially long, I suspect the mouse pointer will be at the end of the details text. Rather than having to scroll back up to collapse the details, they can do so with the appended show less link.

@WithoutPants
Copy link
Collaborator

I believe we should take this opportunity to rename the date heading and its corresponding class name to Release date and release-date respectively. While this item certainly is a date, I think we should specify what the date actually refers to, a [scene] release date. Some people may use it for other purposes, but by and large, all the scrapers are populating a release date.

Hard disagree here. Release date is one interpretation of the date field, but I don't think it's one that should be blanket applied. Users may have studio release, amateur videos or personal content, or a mix of all three and Release date is not a good fit for these. In any case, it's out of scope for this PR.

@echo6ix
Copy link
Contributor

echo6ix commented Feb 23, 2024

Hard disagree here. Release date is one interpretation of the date field, but I don't think it's one that should be blanket applied. Users may have studio release, amateur videos or personal content, or a mix of all three and Release date is not a good fit for these. In any case, it's out of scope for this PR.

That's reasonable. I noticed after posting you also expressed concern for displaying a null field and the preference for a show more/show less functionality for details. Does this mean you're in agreement with the rest of my feedback, namely points 1 and 4.

@WithoutPants
Copy link
Collaborator

Yep, though my preference would be to use <dl>, <dt> and <dd> tags per the SceneFileInfoPanel (using the existing classnames you mentioned), though that might be better done as a separate blanket change for consistency, and it's not really that important now.

@cj12312021
Copy link
Collaborator

cj12312021 commented Feb 28, 2024

I personally think losing the studio image from the detail pages is a step backwards.

I agree with WP about this. The logos are more recognizable than text, so we shouldn't lose that completely.

  1. CSS and Layout

I agree with all of this.

  1. Created at and Updated at dates need styling

I actually like Q's approach to those fields. Those fields are really not the same in that they are not editable by the users. I don't think their style should be the same as the editable fields, and I agree with them being tucked away at the bottom.

I'm not sure I like the rating being presented the same way as the other fields. It currently looks awkward to me and is not handled this way on the other details pages. I haven't thought of a better solution yet.

@echo6ix
Copy link
Contributor

echo6ix commented Feb 29, 2024

I actually like Q's approach to those fields. Those fields are really not the same in that they are not editable by the users. I don't think their style should be the same as the editable fields, and I agree with them being tucked away at the bottom.

This is an arbitrary or subjective reason to break the consistency of the paradigm. Resolution is not editable either; both resolution and creation date are immutable, the updated date will change as often as the user modifies the metadata making it more malleable than the latter in some fashion.

But I do understand what you are communicating and I agree with the implication that different types of metadata should be visually distinct, that is, grouped together, and apart from another.

To me it seems like the simplest, and universal method here, exemplified by guis like Windows File Properties modal and MD3 lists from their style guide, involves visually segmenting different types of metadata. They achieve it through horizontal line separators or header labels or both, to maintain uniformity across their presentation.

Using this principle, this is how I've always presented the concept which this design is derived from. Where all the metadata types are presented cohesively in one tab, grouped by similarity and visually partitioned for clarity. However, this approach wasn't adopted by the author, either because it was deemed outside the scope of what the author wanted to implement, or the finds that principle undesirable.

There is another thread somewhere, where I discuss the logical ordering of the metadata, and this might be considered beyond the scope of this specific commit. But I suspect neglecting a discussion of that broader perspective will lead to situations like this, where small parts of the UI are addressed without a comprehensive consensus or understanding of the ultimate end goal. 🤷

Metadata
- Scene
  - Title
  - Date
  - User rating
  - Tags
  - URLs
  - StashID
  - Scene creation date
  - Scene updated date
- Object (file)
  - Video
    - Resolution
    - Frame rate
    - Bit rate
    - aCodec
    - vCodec
    - Duration
    - Etc
  - File
    - Stream
    - File size
    - Path
    - File modification time
    - Hash
    - pHash
- History
  - Playback
  - O-counter 

Maybe the scene creation and scene updated dates should be grouped with History. But I think that's for another debate once the paradigm/end goal of the presentation has been established.

I'm not sure I like the rating being presented the same way as the other fields [...] and is not handled this way on the other details pages

For consistency with other pages, the only solution is to place the rating stars below the scene title.

@cj12312021
Copy link
Collaborator

cj12312021 commented Feb 29, 2024

This is an arbitrary or subjective reason to break the consistency of the paradigm. Resolution is not editable either; both resolution and creation date are immutable, the updated date will change as often as the user modifies the metadata making it more malleable than the latter in some fashion.

I feel the same way about the resolution. It shouldn't be presented on the tab. I've had CSS hiding it locally on my end for so long that I forgot it was ever a field on that tab. That information already lived in the files tab and was unnecessarily duplicated.

Moving the created and updated date info to the file to a different tab crossed my mind, but I'm personally still happy with Q's approach. That information does not need to be front and center.

For consistency with other pages, the only solution is to place the rating stars below the scene title.

That's a good point. If we are going to utilize the space below the title, we may as well place the o-counter and organized button on that row as well, so the scene title doesn't need to be ellipsed.

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.

None yet

5 participants