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 sidebar full state #456

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Fix sidebar full state #456

merged 1 commit into from
Apr 15, 2020

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Apr 5, 2020

Fix full state Of sidebar

So, I really dislike this. @ChristophWurst @juliushaertl how would you do this? Let the Sidebar in files listen to an event? 🤷‍♀️

@skjnldsv skjnldsv self-assigned this Apr 5, 2020
@skjnldsv skjnldsv added 3. to review Waiting for reviews bug Something isn't working design Related to the design labels Apr 5, 2020
@cypress
Copy link

cypress bot commented Apr 5, 2020 •
edited
Loading


Test summary

151 • 17 • 0 • 0

Run details
Project viewer
Status Failed
Commit 0095491 ℹ️
Started Apr 15, 2020 11:05 AM
Ended Apr 15, 2020 11:08 AM
Duration 02:23 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 78

View run in Cypress Dashboard ➡️

Failures

images-custom-list.spec.js ⏯ 1 Failed
1 Open custom images list in viewer > Does not see a loading animation
sidebar.spec.js ⏯ 9 Failed
1 Open the sidebar from the viewer and open viewer with sidebar already opened > Does not see a loading animation
2 Open the sidebar from the viewer and open viewer with sidebar already opened > Open the sidebar
3 Open the sidebar from the viewer and open viewer with sidebar already opened > Does not have any visual regression 2
4 Open the sidebar from the viewer and open viewer with sidebar already opened > Change to next image with sidebar open
5 Open the sidebar from the viewer and open viewer with sidebar already opened > Does not have any visual regression 3
6 Open the sidebar from the viewer and open viewer with sidebar already opened > Change to previous image with sidebar open
7 Open the sidebar from the viewer and open viewer with sidebar already opened > Does not have any visual regression 4
8 Open the sidebar from the viewer and open viewer with sidebar already opened > Close the sidebar
9 Open the sidebar from the viewer and open viewer with sidebar already opened > Open the viewer with the sidebar open
This comment includes only the first 10 test failures. See all 17 failures in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@cypress
Copy link

cypress bot commented Apr 5, 2020


Test summary

161 • 0 • 0 • 0

Run details
Project viewer
Status Passed
Commit 2c3a165
Started Apr 5, 2020 7:32 PM
Ended Apr 5, 2020 7:34 PM
Duration 01:50 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 78

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@ChristophWurst
Copy link
Member

So, I really dislike this. @ChristophWurst @juliushaertl how would you do this? Let the Sidebar in files listen to an event? woman_shrugging

Is this related to what @ma12-co and you did last Friday?

@skjnldsv
Copy link
Member Author

skjnldsv commented Apr 7, 2020

Is this related to what @ma12-co and you did last Friday?

Not at all, this is how we use to do before the sidebar revamp of 18.
There was no global events, nor proper sidebar back then 🤔

@ChristophWurst
Copy link
Member

Okay, so what do we need to fix here then?

@skjnldsv
Copy link
Member Author

skjnldsv commented Apr 7, 2020

Okay, so what do we need to fix here then?

well, ideally a better way that manually applying some css to the sidebar and overriding style. Does it make sense to have a way to call the sidebar (in server) in full state? Even if maybe only this app uses it?

@ChristophWurst
Copy link
Member

call the sidebar (in server) in full state

What does that mean?

Could you link or describe the problem you're trying to solve please? :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Apr 7, 2020

What does that mean?
Could you link or describe the problem you're trying to solve please? :)

Sure!
Quite simple, when in the viewer, we open the sidebar with 100% full height and over the header so it looks more clean. (we also are supposed to hide the preview, but can't get it to work yet on the new sidebar with this approach).

@ChristophWurst
Copy link
Member

Okay, then I would still say the css approach is okay-ish, right?

@skjnldsv
Copy link
Member Author

skjnldsv commented Apr 7, 2020

Okay, then I would still say the css approach is okay-ish, right?

Not really. I really don't like overriding styles.
Also we can't hide the preview anymore as this requires a prop change in vue.

@ChristophWurst
Copy link
Member

Then I guess it's complicated. Because the sidebar is not a component rendered by this app. Otherwise we could just pass in the full screen info as boolean flag. We might also have to send this info via an event. The question is just if there is someone who listens as that depends on the order the script are loaded.

@skjnldsv
Copy link
Member Author

/compile amend /

@skjnldsv
Copy link
Member Author

/backport to stable18

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label Apr 15, 2020
@skjnldsv @npmbuildbot
Fix sidebar full state
c840783 
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@skjnldsv skjnldsv merged commit b16ba3a into master Apr 15, 2020
@skjnldsv skjnldsv deleted the fix/sidebar-full branch April 15, 2020 13:21
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Apr 15, 2020
Merged
@backportbot-nextcloud
Copy link

backport to stable18 in #470 with conflicts ⚠️

@backportbot-nextcloud backportbot-nextcloud bot removed the backport-request Pending backport by the backport-bot label Apr 15, 2020
@rullzer rullzer mentioned this pull request Apr 15, 2020
Merged
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@danxuliu danxuliu Awaiting requested review from danxuliu

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews bug Something isn't working design Related to the design
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@skjnldsv @ChristophWurst