Fix sidebar layout in compact mode #1338

raimund-schluessler · 2020-08-21T14:14:43Z

Fixes the sidebar layout in compact mode, implements the second part of #1366:

@skjnldsv Since there are many valid combinations of options (compact, with-star, with-header-figure, with-secondary-actions), it needs a good test to find any possibly remaining issues.

raimund-schluessler · 2020-08-21T14:41:02Z

Had to fix an issue, there are just to many different combinations 🙈

skjnldsv · 2020-08-21T17:24:36Z

src/components/AppSidebar/AppSidebar.vue

@@ -597,40 +599,35 @@ $top-buttons-spacing: 6px;
 margin: 0 10px;
 }

- &--compact {
+ &--compact.app-sidebar-header--with-figure {


Hum, this seems like a mistake.
I thought you were asking for files, but it's not general, you can have a big one without a figure. 🤔

Otherwise the compact mode should be just a computed like hasFigure

Well, but if the sidebar has no figure, the compact mode has no effect, right? compact just makes the figure appear smaller on the left of the title instead of large above it.

Then it doesn't make sense to add this class anyway, the &--compact is enough.

No, the compact css should only be applied if a figure is present. If there is no figure and you apply the compact css, there will be issues, since the star is wrongly aligned then. I will check if the compact mode had an effect without a figure before my changes.

My point is why complicate the code then? It's unrelated to this pr. We're targetting the compact mode on this css section, this have nothing to do with app-sidebar-header--with-figure.
Let's not mix template conditions and css structure please :)

Hm, the CSS might be slightly simpler, but we would duplicate all other code and markup just for the sake of getting rid of rules for 4 selectors.

Not if you just create Wrapping components. Single slot.
You'd just externalise the css into two components.

I mean whatever we choose, the whole layout needs to be fixed to what it was before.
What troubles me is that we completely broke something that was working and I can't even grasp the code structure anymore.

I was definitely not planning to spend time on this for the next incoming days/week with the upcoming beta/rc and now server is broken because of this and we released 3 minor versions that now ships a broken AppSidebar. So I'm not super happy about the current situation :)

Yes, I can imagine that, I am sorry I caused this issue.

To my best knowledge this PR and #1337 fix all layout issues you mentioned, so it should repair the sidebar for server.

Since you didn't name any difference the compact mode makes when the sidebar has no figure, I assume that compact mode without figure equals normal mode and created a computed property for this so the --compact selector is not bound to --figure anymore.

I am sorry I caused this issue.

No need to be sorry! We should have reviewed better! You're not alone collaborating there! 🤗
That's why I used we and not you! 😉

And if the code structure is fucked up in your opinion, we can go back to the state it had with vue-components@2.3.0.

All changes to the appsidebar aren't used at the moment anyway, because I just implemented them for the next release of Tasks. And 2.4.0 is only 5 days old, so probably no one makes use of the changes already.

skjnldsv

Main menu in expanded mode is stuck to the right, it shoudl have a right: right: $top-buttons-spacing;
The compact mode selector seems bound to the figure, it feels weird

Also, I'll feel honest, the whole css feels like a mess now 🙈
It wasn't perfect before, but now that this was changed with another logic, I fail to understand it (thus was already hard to remember what was in my mind back then 😁 )

How could we improve?
Maybe split the header with two sub-components? One compact one expanded?

raimund-schluessler · 2020-08-21T17:37:09Z

Maybe split the header with two sub-components? One compact one expanded?

Hm, not sure. After all I think both modes are quire similar. As far as I understand it, compact and normal only differ by the figure size/position (#1338 (comment)).

Once we have the correct behavior with all modes and options, I can try to clean up the css. At the moment I still struggle to understand what the correct / desired behavior actually is tbh.

raimund-schluessler · 2020-08-21T17:41:38Z

Main menu in expanded mode is stuck to the right, it shoudl have a right: right: $top-buttons-spacing;

Yeah, I see. So it should have the same space to the right as the close button. Do you also want to have space between the close button and the action in case of no figure?

raimund-schluessler · 2020-08-21T20:21:09Z

@skjnldsv I cleaned up the sidebar css and structure. To be honest, it feels more clean to me now (but I am for sure biased 🙈). It uses less absolute positioning and relies on flex now. Only the actions / menus are absolutely positioned in some cases.

I hope that I understood what the sidebar is actually supposed to do. Let me know if something behaves not as it should.

skjnldsv · 2020-08-21T20:50:44Z

Yeah, I see. So it should have the same space to the right as the close button. Do you also want to have space between the close button and the action in case of no figure?

you should not be able to have both focused at the same time.
Only the action menu in expanded mode need to have the background as grey by default. In compact it stays as is, transparent :)

raimund-schluessler · 2020-08-21T22:38:11Z

Yeah, I see. So it should have the same space to the right as the close button. Do you also want to have space between the close button and the action in case of no figure?

you should not be able to have both focused at the same time.

Only the action menu in expanded mode need to have the background as grey by default. In compact it stays as is, transparent :)

I didn't change anything in that regard. I think it just looks like this, because I hovered the close button.

skjnldsv · 2020-08-21T22:41:06Z

I didn't change anything in that regard. I think it just looks like this, because I hovered the close button.

Then it's also a regression from the previous pr. This was part of the original specs :)

raimund-schluessler · 2020-08-22T05:58:30Z

I didn't change anything in that regard. I think it just looks like this, because I hovered the close button.

Then it's also a regression from the previous pr. This was part of the original specs :)

Sorry, if I wasn't clear. The menu is currently focused, since I opened and closed it, and the close button is hovered by the mouse. That's why both have a gray background. I think this is how it was before already, but I will double check.

skjnldsv · 2020-08-22T06:37:44Z

Sorry, if I wasn't clear. The menu is currently focused, since I opened and closed it, and the close button is hovered by the mouse. That's why both have a gray background. I think this is how it was before already, but I will double check.

Aaaah, then it's fine! 😝

raimund-schluessler · 2020-08-22T11:11:17Z

@skjnldsv Ok, I was wrong. The menu always has a gray background. But it was like that with vue-components@2.3.0 already (before I changed anything):

Only in compact mode it has no gray background. Should I remove the gray background?

raimund-schluessler · 2020-08-22T11:43:35Z

Some visual regression testing would be nice here, but it seems quite complicated to set up (at least i didn't really managed to do so, the CSS style wouldn't get applied).

skjnldsv · 2020-08-22T12:06:24Z

Only in compact mode it has no gray background. Should I remove the gray background?

No this is what I was saying 😛

Only the action menu in expanded mode need to have the background as grey by default. In compact it stays as is, transparent :)

It's good as it is then. Transparent in compact, grey bg when expanded! 👌

skjnldsv · 2020-08-22T12:07:28Z

Some visual regression testing would be nice here, but it seems quite complicated to set up (at least i didn't really managed to do so, the CSS style wouldn't get applied).

Cypress works fine. We could do that with https://github.com/bahmutov/cypress-vue-unit-test

raimund-schluessler · 2020-08-22T12:17:11Z

Some visual regression testing would be nice here, but it seems quite complicated to set up (at least i didn't really managed to do so, the CSS style wouldn't get applied).

Cypress works fine. We could do that with https://github.com/bahmutov/cypress-vue-unit-test

I will have a look.

skjnldsv · 2020-08-24T10:38:48Z

src/components/AppSidebar/AppSidebar.vue

@@ -338,6 +342,9 @@ export default {
 hasFigureClickListener() {
 return this.$listeners['figure-click']
 },
+ isCompact() {
+ return this.hasFigure && this.compact


Okay, so, sorry about that, but I actually thought about this and I forgot one edge case.
One future addition is to go compact when scrolling down the sidebar. It will be cleaner for super long list like sharing can be.

Thus maybe leaving this to just the compact prop would be better?
What do you think?

I don't mind, we can leave it to the prop.

skjnldsv

Css is much cleaner now :)

Menu in expanded still misses the right alignment like close
Thumbnail is not square anymore

raimund-schluessler · 2020-08-24T12:17:21Z

@skjnldsv Given the huge number of valid combinations of props and slots for the component, I am quite afraid that I will break something else when fixing the issues you mentioned. Since #1339 seems to basically work now, I would like to take screenshots of the AppSidebar component for every combination we have before continuing here. We could then identify the combinations with layout issues and make sure that we don't introduce new problems when fixing the issues found. I will try to do so in the evening.

skjnldsv · 2020-08-24T12:25:49Z

@skjnldsv Given the huge number of valid combinations of props and slots for the component, I am quite afraid that I will break something else when fixing the issues you mentioned. Since #1339 seems to basically work now, I would like to take screenshots of the AppSidebar component for every combination we have before continuing here. We could then identify the combinations with layout issues and make sure that we don't introduce new problems when fixing the issues found. I will try to do so in the evening.

I'm fine doing this in a follow up yes :)

skjnldsv

Please move to just compact and rebase :)

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>

raimund-schluessler · 2020-08-24T13:48:07Z

Please move to just compact and rebase :)

Done.

I cannot reproduce the issues you described in #1338 (review) by the way. To me it looks fine. Are you sure you tested the latest state?

skjnldsv · 2020-08-24T14:00:36Z

Are you sure you tested the latest state?

I think I pulled yes, let's get this in and see what comes next :)

raimund-schluessler · 2020-08-24T14:04:31Z

I will work on #1339 in the evening. Hopefully we can then check all different options properly.

raimund-schluessler added bug Something isn't working 3. to review Waiting for reviews labels Aug 21, 2020

raimund-schluessler requested review from georgehrke and skjnldsv August 21, 2020 14:14

skjnldsv mentioned this pull request Aug 21, 2020

Allow unified search filtering and bump @nextcloud/vue nextcloud/server#22111

Merged

skjnldsv reviewed Aug 21, 2020

View reviewed changes

skjnldsv requested changes Aug 21, 2020

View reviewed changes

skjnldsv linked an issue Aug 22, 2020 that may be closed by this pull request

Issues after AppNavigation update #1288 #1336

Closed

3 tasks

raimund-schluessler force-pushed the fix/1366/layout branch from c9f1819 to c3fbbc0 Compare August 22, 2020 13:16

raimund-schluessler mentioned this pull request Aug 23, 2020

Add visual regression testing with cypress #1339

Closed

skjnldsv reviewed Aug 24, 2020

View reviewed changes

skjnldsv requested changes Aug 24, 2020

View reviewed changes

skjnldsv approved these changes Aug 24, 2020

View reviewed changes

raimund-schluessler force-pushed the fix/1366/layout branch from c3fbbc0 to 3bf89c3 Compare August 24, 2020 12:57

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 Aug 24, 2020

Fix sidebar layout in compact mode

c731c61

Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>

raimund-schluessler force-pushed the fix/1366/layout branch from 3bf89c3 to c731c61 Compare August 24, 2020 13:47

raimund-schluessler requested a review from skjnldsv August 24, 2020 13:48

skjnldsv approved these changes Aug 24, 2020

View reviewed changes

raimund-schluessler merged commit 394d72f into master Aug 24, 2020

raimund-schluessler deleted the fix/1366/layout branch August 24, 2020 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sidebar layout in compact mode #1338

Fix sidebar layout in compact mode #1338

raimund-schluessler commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

skjnldsv Aug 21, 2020

raimund-schluessler Aug 21, 2020

skjnldsv Aug 22, 2020

raimund-schluessler Aug 22, 2020

skjnldsv Aug 22, 2020

skjnldsv Aug 22, 2020

skjnldsv Aug 22, 2020

raimund-schluessler Aug 22, 2020

skjnldsv Aug 22, 2020

raimund-schluessler Aug 22, 2020

skjnldsv left a comment •

edited by raimund-schluessler

Loading

raimund-schluessler commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

skjnldsv commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

skjnldsv commented Aug 21, 2020

raimund-schluessler commented Aug 22, 2020

skjnldsv commented Aug 22, 2020

raimund-schluessler commented Aug 22, 2020

raimund-schluessler commented Aug 22, 2020

skjnldsv commented Aug 22, 2020

skjnldsv commented Aug 22, 2020

raimund-schluessler commented Aug 22, 2020

skjnldsv Aug 24, 2020

raimund-schluessler Aug 24, 2020

skjnldsv left a comment

raimund-schluessler commented Aug 24, 2020

skjnldsv commented Aug 24, 2020

skjnldsv left a comment

raimund-schluessler commented Aug 24, 2020

skjnldsv commented Aug 24, 2020

raimund-schluessler commented Aug 24, 2020

Fix sidebar layout in compact mode #1338

Fix sidebar layout in compact mode #1338

Conversation

raimund-schluessler commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv left a comment • edited by raimund-schluessler Loading

Choose a reason for hiding this comment

raimund-schluessler commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

skjnldsv commented Aug 21, 2020

raimund-schluessler commented Aug 21, 2020

skjnldsv commented Aug 21, 2020

raimund-schluessler commented Aug 22, 2020

skjnldsv commented Aug 22, 2020

raimund-schluessler commented Aug 22, 2020

raimund-schluessler commented Aug 22, 2020

skjnldsv commented Aug 22, 2020

skjnldsv commented Aug 22, 2020

raimund-schluessler commented Aug 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv left a comment

Choose a reason for hiding this comment

raimund-schluessler commented Aug 24, 2020

skjnldsv commented Aug 24, 2020

skjnldsv left a comment

Choose a reason for hiding this comment

raimund-schluessler commented Aug 24, 2020

skjnldsv commented Aug 24, 2020

raimund-schluessler commented Aug 24, 2020

skjnldsv left a comment •

edited by raimund-schluessler

Loading