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

[RFC] Revamp scene page #562

Merged
merged 32 commits into from
Jun 18, 2020

Conversation

WithoutPants
Copy link
Collaborator

My primary goal for this was to fix an annoyance I had when reviewing and adding info to multiple scenes. Having to scroll away from the video to enter tags and performers was annoying.

Inspiration for this change comes from this comment in #108

Summary of changes:

  • move scene details to the left side of the screen on large (>1200px width) screens
    ** show the scene title and studio logo above the tabs on large screens
  • set the scene player/scrubber height to the height of the viewport (allowing for navbar)
  • hide the scrubber on screens < 450px height
  • move performers from dedicated tab into the details tab
  • move operations from dedicated tab into a dropdown button in the tab bar
  • move the edit buttons to the top of the panel from the bottom

This is a highly subjective change, so feedback is welcomed. If this change is too contentious, I can look at a popout button to toggle between the left-docked and bottom views.

Screenshots:

(large screen)

image

image

(smaller screen)

image

(smallest before scrubber is removed)
image

(scrubber removed when viewport height < 450px)
image

(operation dropdown)
image

@WithoutPants WithoutPants added the improvement Something needed tweaking. label May 20, 2020
@WithoutPants
Copy link
Collaborator Author

TODO: remove tsx files that are now redudant

@bnkai
Copy link
Collaborator

bnkai commented May 20, 2020

I like most of the changes.
Personal highly subjective preference :-) below:

The only thing that feels weird for me is the operations that seems hidden ( i think it's just because i was used to seeing it in its old position. the ... vs operations doesn't help also. Maybe add to details as buttons next to/ above / below save delete? )
Also the performer images ( when there is more than one in a scene looked better the "old way" )

@echo6ix
Copy link
Contributor

echo6ix commented May 20, 2020

Huge improvement.

  • smaller views
    • i like that you're purging the scrubber from view when the display is smaller. less is more
  • operations drop down
    • really like this idea
  • tooltips
    • consider giving the operations drop down icon and counter icon a tooltip; users won't have to ask or guess at what they do
  • studio logo
    • the studio logo seems large and prominent, but like you said it's subjective. personally i've never been a huge fan of the logo on the scenes page
    • i think on smaller viewports you might want to consider not displaying the graphic at all and instead just the studio text? or...
  • the title text is obviously important and should remain prominent. an alternative to consider is displaying the studio name and date directly below it (in a smaller font size and weight), such as By {studio name} • {date} kind of like youtube does in the attached image below:

Annotation 2020-05-20 191713

  • details panel
    • imo all the info in the details panel should be visually consistent, and tabular in nature like the edit panel.
    • all fields and values with the same font size and weight
    • perhaps even separating each field with a very subtle horizontal line as seen in material design themes to give it more consistency, thought i'm not sure how that would look on this since it's still very much blueprint
    • if the scene has a url assigned to it, shouldn't it be displayed (as a hyperlink) on this part of the page since it's basically a source/citation of all the scene detail

@ghost
Copy link

ghost commented May 21, 2020

Great work, it's a huge improvement I think. I do have some feedback though:

  • I think we should shrink the performer cards a bit. Creating a small variant with width 15rem and height 22.5rem gives room for two cards per row, which gives a better overview. The flag also needs to be shrunk a bit to fit.

image

  • The player is currently 100% height when in narrow mode, which fills the screen with black bars, and breaks the layout on mobile. IMO it should follow the height of the video.

  • I suggest hiding the scrubber at mobile size. It's pretty much useless anyway.

  • The studio logo in narrow mode takes up way too much space. The description should be full width, and I would suggest either moving the logo to the right of the title and resolution, or just using text. In this example I set everything to full width, set a max-height of 8rem on the logo, moved it above the title and set float-right.

image

  • In wide mode the input fields are very narrow. I would suggest making at least the tag and performer fields full width like the details field.

@WithoutPants
Copy link
Collaborator Author

  • added tooltips for the operations and o-counter buttons per @echo6ix suggestion

In the xl view:

  • made max-height of the logo 8rem per @InfiniteTF suggestion
  • adjusted performer card sizes per @InfiniteTF suggestion
  • changed edit panel form to use responsive Form.Group rather than a table, allowing fields to move onto the next line at certain screen sized, per @InfiniteTF suggestion

image

image

In non-xl view:

  • adjusted logo and details layout per @InfiniteTF suggestion

image

In mobile view:

  • removed scrubber per @InfiniteTF suggestion
  • adjusted performer card sizes like for xl view

image

I have also added Delete Scene to the operations drop down menu. I considered removing Delete from the Edit tab, but I think this will confuse users, and there's little harm keeping it there. Having it in the operations dropdown means you don't need to change tabs to delete a scene.

To address some things that weren't covered above:

@echo6ix - I think that the details panel probably needs another pass over for improvement, but I'm hesitant to mess with it further within the scope of this PR. I think that the date, rating and resolution fields need further refinement, but I don't have any particular ideas at this point. I agree with including the URL somewhere there, but I think I'll push that out to another PR.

@InfiniteTF - re: video size in mobile view. The layout for the video player has been a bit of a PITA to deal with. For larger views, I wanted it to fill the available height space while leaving room for the scrubber at the bottom. This means setting the jwplayer dimensions manually to 100%, rather than letting it autosize it to the dimensions of the playing video. This does mean that on mobile views, the dimensions are similarly auto-sized. I haven't been able to get it to use the aspect ratio in mobile view, while using a fixed dimension for the larger view. IMO the mobile view looks fine as is, but I understand it's subjective. If you can figure out a way around it, I'm happy to put it in, but I've had no luck so far.

@bnkai I moved operations to the ellipsis dropdown primarily to save space. It seemed wasteful to have an entire tab to provide two buttons. I think it makes for a cleaner interface, but again it's a bit subjective.

@ghost
Copy link

ghost commented May 24, 2020

The following style inside #jwplayer-container in ScenePlayer/styles.scss will return it to the current behavior on smaller screens:

  @media (max-width: 1200px) {
    height: inherit;

    .jw-aspect {
      display: block;
      padding-top: 56.25%;
    }
  }

The 56.25% padding forces the ratio of 16/9 essentially.

Also, the flag overlaps a bit so I would add
.flag-icon { width: 2rem; height: 1.33rem } to the performer card media query.

@WithoutPants
Copy link
Collaborator Author

This issue with the old behaviour was that it was possible for the video player to be taller than the viewport height, and that's the behaviour that I got when I put that I put the above CSS in. Setting the max-height property had no impact. This was the main reason I used fixed sizes for the player, because it seemed impossible to let the player auto-determine its own dimensions while constraining them with max-.

@ghost
Copy link

ghost commented May 25, 2020

This issue with the old behaviour was that it was possible for the video player to be taller than the viewport height, and that's the behaviour that I got when I put that I put the above CSS in. Setting the max-height property had no impact. This was the main reason I used fixed sizes for the player, because it seemed impossible to let the player auto-determine its own dimensions while constraining them with max-.

Right, the padding isn't constrained by the height. If you change padding: 56.25% to height: 56.25vw you can set max-height: 100vh (or calc(100vh - 45px) to account for the navbar) to make sure it sticks to the dimensions

@WithoutPants
Copy link
Collaborator Author

WithoutPants commented May 25, 2020

TODO - the scene title isn't displaying well since I removed the text truncate class.

@WithoutPants
Copy link
Collaborator Author

Fixes #575

@bnkai
Copy link
Collaborator

bnkai commented Jun 15, 2020

From a first test everything seems ok.

@WithoutPants WithoutPants merged commit 3fbb4cd into stashapp:develop Jun 18, 2020
Tweeticoats pushed a commit to Tweeticoats/stash that referenced this pull request Feb 1, 2021
* Don't show scrubber on small height device
* Move operations into ellipsis menu
* Hide scrubber in mobile devices
* Add delete scene to operations drop down
* Remove redundant panels
* Fix video height on smaller devices
* Adjust player aspect ratio for portrait videos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Something needed tweaking.
Projects
None yet
3 participants