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.

Sign up for GitHub

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

Jump to bottom

- fix Photos not shown in large browser windows #630 #664

Closed
wants to merge 3 commits into from
Closed

- fix Photos not shown in large browser windows #630 #664

wants to merge 3 commits into from

Conversation

dsmic
Copy link
Contributor

@dsmic dsmic commented Feb 18, 2021 •
edited by skjnldsv
Loading

As the title says: this should fix this issue, as the max: key was not handled properly before

Fix #630

@dsmic
Copy link
Contributor Author

dsmic commented Feb 18, 2021

Sorry, I don't know how to fix the last missing check, but the code changes are minimal, so you could probably merge it without those changes....

@skjnldsv
Copy link
Member

/compile /

skjnldsv
skjnldsv reviewed Feb 18, 2021
View reviewed changes
src/services/GridConfig.js Outdated
@@ -44,7 +44,7 @@ export default new Vue({
methods: {
handleWindowResize() {
// find the first grid size that fit the current window width
const currentSize = Object.keys(sizes).find(size => size > document.documentElement.clientWidth)
const currentSize = Object.keys(sizes).find(size => size > document.documentElement.clientWidth || size === 'max')
this.gridConfig = sizes[currentSize]
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the fallback on the this.gridConfig assignment instead.

Suggested change
this.gridConfig = sizes[currentSize]
// fallback to max if none matches
this.gridConfig = sizes[currentSize] || sizes['max']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be more elegant, sorry I am not a javascript guy 😊😊

Copy link
Member

Choose a reason for hiding this comment

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

No worries :)

Can you do the change so I can approve? :)

@skjnldsv skjnldsv added 2. developing Work in progress bug Something isn't working labels Feb 18, 2021
@dsmic
Copy link
Contributor Author

dsmic commented Feb 19, 2021 •
edited
Loading

OK, still fighting with the automatic checks.
With your suggestion I got the Error ['max'] better written in dot notation, so I did it :)

Thanks for teaching the power of || in javascript, Detlef

@skjnldsv

This comment has been minimized.

Sign in to view
@skjnldsv
Copy link
Member

/backport to stable21

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label Feb 22, 2021
@skjnldsv
Copy link
Member

/backport to stable20

@skjnldsv
Copy link
Member

/backport to stable19

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 22, 2021
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 22, 2021
@skjnldsv
Copy link
Member

Thanks @dsmic
Next time you open a pr, could you enable the owners of the repo to push to your fork? That help us rebase things without having to wait for you if necessary :)

Because I can't rebase now, please either enable write permissions or rebase yourself this branch to master 😉

@skjnldsv
Copy link
Member

Because I can't rebase now, please either enable write permissions or rebase yourself this branch to master

Ah, my bad, it seems we have the permissions, no idea why my push failed.

@skjnldsv
Copy link
Member

skjnldsv commented Feb 22, 2021 •
edited
Loading

Yeah, I can't rebase and push, @dsmic please rebase, and drop changes on /js so we can update the bundles again :)

@dsmic dsmic closed this Feb 22, 2021
@dsmic
Copy link
Contributor Author

dsmic commented Feb 22, 2021

Sorry, but I don't know, why it closed the pull request. I cleaned my fork ...
In case you still have trouble I added you to the fork...

@dsmic dsmic reopened this Feb 22, 2021
@skjnldsv

This comment has been minimized.

Sign in to view
@skjnldsv
Copy link
Member

Sorry, but I don't know, why it closed the pull request. I cleaned my fork ...

Ah, you didn't rebased to latest master!
We need to ship the compiled files, but if you're not up-to-date, there will be conflicts.

dsmic and others added 2 commits February 22, 2021 13:14
@dsmic
- fix Photos not shown in large browser windows #630
97869ef 
Signed-off-by: detlef <ds2@physik.de>
@npmbuildbot-nextcloud @dsmic
Compile assets
88e4e6e 
Signed-off-by: npmbuildbot-nextcloud[bot] <npmbuildbot-nextcloud[bot]@users.noreply.github.com>
@dsmic
Copy link
Contributor Author

dsmic commented Feb 22, 2021 •
edited
Loading

Am I supposed to commit my changes from the build?

'Zum Commit vorgemerkte Änderungen:
geändert: js/photos-0.js
geändert: js/photos-0.js.map
geändert: js/photos-1.js
geändert: js/photos-1.js.map
geändert: js/photos-10.js
geändert: js/photos-10.js.map
geändert: js/photos-2.js
geändert: js/photos-2.js.map
geändert: js/photos-3.js
geändert: js/photos-4.js
geändert: js/photos-4.js.map
geändert: js/photos-5.js
geändert: js/photos-5.js.map
geändert: js/photos-6.js
geändert: js/photos-6.js.map
geändert: js/photos-7.js
geändert: js/photos-7.js.map
geändert: js/photos-8.js
geändert: js/photos-8.js.map
geändert: js/photos-9.js
geändert: js/photos-9.js.map
geändert: js/photos-main.js
geändert: js/photos-main.js.map
geändert: js/photos-service-worker.js

Unversionierte Dateien:
js/photos-3.js.map
'

@skjnldsv
Copy link
Member

Am I supposed to commit my changes from the build?

Yes, this repo expect npm bundles to be up--to-date before merging

@skjnldsv

This comment has been minimized.

Sign in to view
@dsmic
- added the js package
eb2ee90 
Signed-off-by: detlef <ds2@physik.de>
@dsmic
Copy link
Contributor Author

dsmic commented Feb 22, 2021

I still get node12.x failing:

`Uncommited changes in webpack build
HEAD detached at pull/664/merge
Changes not staged for commit:
(use "git add ..." to update what will be committed)
(use "git restore ..." to discard changes in working directory)
modified: js/photos-0.js
modified: js/photos-0.js.map
modified: js/photos-1.js
modified: js/photos-1.js.map
modified: js/photos-10.js
modified: js/photos-10.js.map
modified: js/photos-2.js
modified: js/photos-2.js.map
modified: js/photos-4.js
modified: js/photos-4.js.map
modified: js/photos-5.js
modified: js/photos-5.js.map
modified: js/photos-6.js
modified: js/photos-6.js.map
modified: js/photos-7.js
modified: js/photos-7.js.map
modified: js/photos-8.js
modified: js/photos-8.js.map
modified: js/photos-9.js
modified: js/photos-9.js.map
modified: js/photos-main.js
modified: js/photos-main.js.map
modified: js/photos-service-worker.js

no changes added to commit (use "git add" and/or "git commit -a")
Error: Process completed with exit code 1.`

@dsmic dsmic closed this Feb 24, 2021
@dsmic dsmic mentioned this pull request Feb 24, 2021
Closed
@skjnldsv skjnldsv mentioned this pull request Feb 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish backport-request Pending backport by the backport-bot bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Photos not shown in large browser windows
2 participants
@dsmic @skjnldsv