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

DocumentManager #93

Merged
merged 15 commits into from
Jan 14, 2021
Merged

DocumentManager #93

merged 15 commits into from
Jan 14, 2021

Conversation

hawken93
Copy link
Contributor

@hawken93 hawken93 commented Dec 12, 2020

Making DocumentManager and taking the functions that deal with DOM in there.

Also fixed:

  • Image preloading
  • less DOM work for ccast audio
  • Size of poster image in details page
  • linting

@hawken93 hawken93 marked this pull request as draft December 12, 2020 17:28
@hawken93
Copy link
Contributor Author

Thanks for the review! I'll get back to it :)

@hawken93
Copy link
Contributor Author

@simonmarklar I've done changes you suggested and I think I'm ready for another review :)

@hawken93 hawken93 force-pushed the document-manager branch 2 times, most recently from 29f6d68 to d716363 Compare December 23, 2020 14:56
@hawken93 hawken93 force-pushed the document-manager branch 3 times, most recently from 7bdd489 to 676008f Compare January 1, 2021 20:30
@hawken93 hawken93 changed the title WIP: DocumentManager DocumentManager Jan 2, 2021
@hawken93 hawken93 marked this pull request as ready for review January 2, 2021 11:08
@hawken93
Copy link
Contributor Author

hawken93 commented Jan 2, 2021

Ready for reviews and regression testing!

Afaik this should behave no differently from master

@ferferga
Copy link
Member

ferferga commented Jan 2, 2021

@hawken93 Not strictly related to this PR but I think it fits to be added here:

Right now we "render" the UI for all devices, even Chromecast Audios. Can you add a check here, so if the device is a Chromecast Audio, a display: none is added to the whole page?

See https://developers.google.com/cast/docs/audio

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 2, 2021

Hmm I could try. I'm thinking to mess with the detection to make it think my gen2 is an audio for test

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 2, 2021

@ferferga I've eased up the DOM operations on cc audio devices :)

@hawken93
Copy link
Contributor Author

hawken93 commented Jan 3, 2021

I have updated the style a bit. I think it looks good :)

@hawken93 hawken93 force-pushed the document-manager branch 4 times, most recently from d6f2080 to 74c5d21 Compare January 14, 2021 01:06
@hawken93
Copy link
Contributor Author

hawken93 commented Jan 14, 2021

Okay @hamburglar2160 requesting new test, I've only deployed this PR right now. There have been a bunch of changes to UI since, so mostly UI related breakage may happen compared to nightly.
UPDATE: also added the misc-fixes PR to my instance

@ferferga @simonmarklar new review?

@hawken93 hawken93 force-pushed the document-manager branch 2 times, most recently from b0e0594 to 2ba05ae Compare January 14, 2021 01:57
@hawken93
Copy link
Contributor Author

I've modified waiting and details pages to use relative scaling, should make 4k display look the same as 720p display and I think it's for the better, @hamburglar2160 opinion?

src/components/deviceprofileBuilder.ts Show resolved Hide resolved
Comment on lines +132 to +133
* @param {BaseItemDto} item to show information about
* @returns {Promise<void>} for the page to load
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc syntax is like this: @param {type} argumentInFunction - Description

@param {type} [argumentInFunction=value] - Description

for arguments that have default values.

Non-blocking though, I can fix that in the linting PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see the hyphen is optional and just a readability thing, but from this I can fix the other one

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Assumming this was fully tested after the new changes, this LGTM

@ferferga ferferga merged commit 50e49c8 into jellyfin:master Jan 14, 2021
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.

5 participants