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

Quickfix for sidebar on NC14 #885

Merged
merged 1 commit into from
Sep 5, 2018
Merged

Quickfix for sidebar on NC14 #885

merged 1 commit into from
Sep 5, 2018

Conversation

tcitworld
Copy link
Member

Closes #876

I didn't test on NC < 14, but it should be fine since it only puts back
old sidebar behaviour before nextcloud/server#10218

@tcitworld tcitworld added 3. to review Waiting for reviews bug labels Aug 6, 2018
@tcitworld
Copy link
Member Author

tcitworld commented Aug 6, 2018

Maybe @skjnldsv can leave some input on how to handle position: sticky; better instead of returning to position: fixed as I did ? :)
The issue is that the editor panel is inside a popup over the content, not aside it.

@skjnldsv
Copy link
Member

skjnldsv commented Aug 6, 2018

@tcitworld sure, position sticky is the same as fixed instead in is based on the parent scrolling position.
Currently, the first scrollable parent is body, so you just need to make sure you don't add any overflow to any parent the sticky element have.

Nonetheless, since the calendar don't have any scrollable area? (right?)
Sticky shouldn't change anything I think. Let me have a look :)

@skjnldsv
Copy link
Member

skjnldsv commented Aug 6, 2018

Ah, the layout is not ok, the sidebar is supposed to be right next to the content
nextcloud/server#9982

@tcitworld
Copy link
Member Author

Nonetheless, since the calendar don't have any scrollable area? (right?)

Will have to check on that. :/

Ah, the layout is not ok, the sidebar is supposed to be right next to the content

That's the issue I mentioned earlier with

The issue is that the editor panel (sidebar) is inside a popup over the content, not aside it.

I think it's because reducing the calendar's width makes it quite ugly that it's like that. Do you think the sidebar panel can be put aside and still get over the main content ?

I'll try to have a look on how other apps did with nextcloud/server#9982.

@skjnldsv
Copy link
Member

skjnldsv commented Aug 6, 2018

@tcitworld the calendar is a bit aside from the others app :)
Maybe just add a manual fixed override like you did then.
Just comply to the first layout where only app-content is needed and not the #app one :)
See the contacts hack we did (since the new update won't have the same behavior, this is just a quick fix) https://github.com/nextcloud/contacts/blob/master/js/contacts-inject-14.js

@tcitworld
Copy link
Member Author

Need to review after #882 is merged.

Closes #876

I didn't test on NC < 14, but it should be fine since it only puts back
old sidebar behaviour before nextcloud/server#10218

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
@georgehrke
Copy link
Member

rebased

Copy link
Member

@georgehrke georgehrke left a comment

Choose a reason for hiding this comment

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

The position itself is fixed, but for me Friday, Saturday and Sunday are hidden behind the sidebar now. The calendar-grid should adjust its width

@georgehrke
Copy link
Member

@skjnldsv Do you know what change in the server caused this?
The behavior in the files app is correct. (if sidebar opens, width of filelist adapts)

@skjnldsv
Copy link
Member

@georgehrke because the sidebar is not fixed anymore. It's inline flexed with the content.
If you do this, the calendar will be under the sidebar! :)
Which is OK of course if that's what you want! ;)

@skjnldsv
Copy link
Member

Imho i feel like it's easier to always see the content. Let say for example that you're editing an event but want to check another one on the next sunday to make sure you're available this date. :)

@georgehrke
Copy link
Member

@skjnldsv Would you mind looking into this? It's probably just two or three lines of css.

@skjnldsv skjnldsv self-assigned this Aug 21, 2018
@skjnldsv
Copy link
Member

skjnldsv commented Sep 5, 2018

It's probably just two or three lines of css.

It's never that easy 😁
The app sidebar is not on the right location according to the standard, but seeing the overall layout, it would be far too hard to change everything.
This patch works fine, let's merge this for 14 and make sure the update to vue fit the new structure guidelines.

@skjnldsv skjnldsv dismissed georgehrke’s stale review September 5, 2018 07:56

Please update review again :)

@georgehrke
Copy link
Member

Roger that!

@georgehrke georgehrke merged commit 4d2a9fc into master Sep 5, 2018
@georgehrke georgehrke deleted the fix-calendar-nc-14 branch September 5, 2018 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants