-
-
Notifications
You must be signed in to change notification settings - Fork 803
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
Details redesign tweaks and refactoring #3995
Details redesign tweaks and refactoring #3995
Conversation
For what it's worth, I've always thought of it as "StashIDs" as well. It feels like that's been the most commonly used spelling too, both within the UI and in conversation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't run into any issues with the changes during my testing. I suspect the URL changes you made will break a lot of user scripts, but they can be updated.
import { useEffect } from "react"; | ||
|
||
export function useScrollToTopOnMount() { | ||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. It wasn't this solution that it sunk it that useEffect's run on render.
ui/v2.5/src/components/Performers/PerformerDetails/Performer.tsx
Outdated
Show resolved
Hide resolved
@@ -213,7 +213,7 @@ export const PerformerDetailsPanel: React.FC<IPerformerDetails> = ({ | |||
fullWidth={fullWidth} | |||
/> | |||
<DetailItem | |||
id="StashIDs" | |||
id="stash_ids" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually under the impression that StashID wasn't meant to be translated. I didn't realize a location string existed for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only noticed because I was getting a console error about a missing intl string - react-intl
's behaviour of just showing the ID when no intl string exists isn't meant to actually be used, it's just a fallback so that at least something is shown.
a687ee3
to
08e002f
Compare
08e002f
to
7bfa8a1
Compare
These are a few tweaks and changes related to the recent details redesign, with some refactoring thrown in as well.
onscroll
event. This fixes an issue I came across where the sticky header would flicker if the page's initial scroll position was such that the header should show.en-GB.json
translation file has "Stash IDs" but the hardcoded strings all were "StashIDs". All the labels are now consistently display "Stash IDs", but I feel like "StashIDs" looks better - although maybe that's simply because I've seen "StashIDs" more often.window.scrollTo(0, 0)
in theGridCard
onclick
event and instead scrolling up to the top when a "long" component (studios, tags, etc pages, list pages) is mounted. ThescrollTo
in theonclick
was causing the page to unconditionally scroll up when clicking on a card, even if the click doesn't actually navigate away to a new page (eg a middle click to open in a new tab).id
to thecover
field ofSlimGalleryData
, thus allowing the field to be properly cached, which fixes a console warning complaining about the missing ID. I also set the merge function for thepaths
field of aScene
object tofalse
, fixing another console warning, and removed a few unnecessaryfalse
merge functions on fields which return normalized objects and thus do not need them.useTitleProps
andmakeTitleProps
, and made it more modular with support for multiple pipe-separated parts.SceneLoader
,PerformerLoader
, etc) and created anImageLoader
to directly use theRouteComponentProps
passed to them byreact-router
instead ofuseLocation
oruseParams
.Refactored the handling of the "tab" URL parameter, adding aTabKey
type. Now instead of having/performer/11
display the performer's scenes, the page will be redirected to/performer/11/scenes
first before showing the scenes. This allows for a possible future feature where you would be able to set the default tab instead of it being hardcoded. Existing URL query strings are retained by the redirection, retaining backwards-compatibility:/performers/1?sortby=date
will redirect to/performers/1/scenes?sortby=date
for example.Edit: This is now just a refactor of the tab handling - the behaviour should match how it was before. I'll leave that change for a subsequent PR implementing [Feature] Global settings option to set the default opened nav-tab (for Performer, Tag, Studio pages) #3961.
classnames
to generate the class string for the.detail-header
instead of doing it manually with a template string - it just looks a bit cleaner.This ended up being a lot bigger than I had anticipated, because I kept stumbling across things to refactor and couldn't help myself. I'm happy to split anything out into separate PRs if necessary.