[WIP] Fit to styleguide, closes #49 #51

raimund-schluessler · 2017-02-18T17:59:24Z

Superseded by these PRs:

Fix input fields: Fix task input fields #52
Fix app navigation menu, delete countdown and sort order dropdown menu: Fix app navigation menu, closes #49 #53
Fix button border radius: Fix button border radius #54

raimund-schluessler · 2017-02-19T23:15:18Z

I fixed all issues I found and tried to fit the guidelines as far as possible. The only thing left is the positioning of the popover menu under the three-dots-icon which is slightly off.

@skjnldsv Could you maybe have a look at this? I thought the popover menu would be positioned automatically when it is located within the div containing the three-dots-icon?

skjnldsv · 2017-02-20T04:28:50Z

templates/main.php

-                    <ul>
-                        <li class="app-navigation-entry-utils-counter">{{ getCollectionString(collection.id) | counterFormatter }}</li>
-                    </ul>
+                <div class="app-navigation-entry-utils-counter">


You need the <div class="app-navigation-entry-utils"> to fit the css guidelines.
https://docs.nextcloud.com/server/11/developer_manual/app/css.html#menus

Ah, alright, thanks for the link to the manual. Would it be allowed to have the counter on the second position so it doesn't get shifted when the three-dots-icon is visible?

skjnldsv · 2017-02-20T04:36:41Z

templates/main.php

-                        <li confirmation="delete(calendar)" confirmation-message="deleteMessage(calendar)">
-                        </li>
-                    </ul>
+                <div class="app-navigation-entry-utils-counter">


Should be first. Before the app-navigation-entry-menu and the app-navigation-entry-utils-menu-button button.

And it's empty on my installation, therefore moving the menu icon to the left.

Yeah, I notived that too. I will align the thee-dots-icon to the right in case no counter is visible.

No need to do that, it should align itself if at the right place :)

Yes, but see my comment above. I would like to switch the places of counter and three-dots-icon, but I guess this is against the guidelines !?

Sorry, I thought you said that you will align it, not that you wanted to :)
Well, I don't think this is against the guidelines, but if you hide the counter properly, the menu should align automatically on the right :)

I will take a look asap 😉

skjnldsv · 2017-02-20T04:39:51Z

Nice work! Still a shame that you need to add your own css to the popover menu for the countdown to work. But the countdown works fine now. 😃

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2017-02-20T16:27:58Z

@raimund-schluessler I pushed a pr to fit the css guidelines.
The only thing to fix is the menu now. :)

It handles your count alignment. (Though it looks a little bit strange, but that what you asked 😉 )
I fixed your strange countformatter filter
I fixed some css to avoid too much !important flaws

skjnldsv · 2017-02-20T16:35:28Z

Okay, I looked at the overall changes you did and found some strange stuff.
Why did you change the menu/menubutton div order? The app was fitting the css guidelines before, the popovermenu content didn't. There was no needs for that didn't it?

raimund-schluessler · 2017-02-20T18:08:45Z

Thanks for fixing this stuff. I haven't checked how it looks now, but I will do so soon.

Why did you change the menu/menubutton div order? The app was fitting the css guidelines before, the popovermenu content didn't. There was no needs for that didn't it?

What exactly do you mean? Could you give a link to the line in the code?

skjnldsv · 2017-02-20T18:26:22Z

Well, if you look at my pr and the overall changes, I pretty much rolled back to the original code (without knowing it). That means the structure of the app-navigation was good on the first place. So why did you changed it? The only stuff that was necessary to change was the popover menu with the countdown, why editing the rest?

raimund-schluessler · 2017-02-20T19:01:56Z

Yeah, you are right. I guess I screwed that up in an attempt to fix the positioning of the three-dots-icon without realising. Thanks for reverting that.

But there are some new issues now.

The counter is not always visible, only when the list is active. It should always be visible.
The counter now shows 0 when no tasks are present, whereas it simply showed nothing before. I know, the counterFormatter in the guidelines does the same, but I think it clutters the layout with a useless display.

I will fix the first issue and would like some feedback on the second. Also @jancborchardt for the second point.

Also the popover menu does not open, when clicking on the three-dots-icon

raimund-schluessler · 2017-02-20T19:07:07Z

Wait, you moved the popover menu again outside of the div containing the three-dots-icon. But in nextcloud/server#2798 (comment) you say

You need to put the entire menu just after the three dot icon (refs #2545 (comment))

<div><span class="icon-more"></span><div class="popover">...</div></div>
With this, we don't need js, css only is ok.

Also see nextcloud/server#2545 (comment)

For référence: the popover menu should be inside the div containing the dots icon.

What is correct now?

skjnldsv · 2017-02-20T19:09:35Z

Okay, I understand the confusion now! :)
I wrote the guide for the popover menu only. Not for the app-navigation menu.

The css guidelines on the dev manual still apply there. The only stuff that changed which is the same for the two menus is the internal structure of elements. (span, divs...)

raimund-schluessler · 2017-02-20T19:11:16Z

Ah, ok, thanks for the insight 😄

I will fix the issues then.

skjnldsv · 2017-02-20T19:11:58Z

We should probably revert all of this pr and only fir the countdown ^^'

Sorry about the confusion, the dev manual not beiing updated is pretty annoying (don't have time for that yet 😢 )

raimund-schluessler · 2017-02-20T19:16:18Z

Well, the countdown is not the only thing that had to be changed. There actually were several issues including opacity, the sortorder dropdown and the input field.

But starting a new PR and applying these fixes there might be an option.

jancborchardt · 2017-05-22T17:26:10Z

Great work on fitting the app to the styleguide! :)

Fix popover menu

9ea08d8

raimund-schluessler changed the title ~~[WIP] Fix popover menu, fixes #49~~ [WIP] Fix popover menu, closes #49 Feb 18, 2017

raimund-schluessler changed the title ~~[WIP] Fix popover menu, closes #49~~ [WIP] Fit to styleguide, closes #49 Feb 18, 2017

Raimund Schlüßler added 5 commits February 19, 2017 17:59

Fix sortorder dropdown

4ad7cf6

Fix positioning of popover menu

b3c6829

Fix width of popover labels

9a30955

Fix input fields

d9d43fc

Fix button border radius

00c5e1b

raimund-schluessler changed the title ~~[WIP] Fit to styleguide, closes #49~~ Fit to styleguide, closes #49 Feb 19, 2017

raimund-schluessler mentioned this pull request Feb 19, 2017

Deletion countdown cancelation not working since the last popover unification #49

Closed

3 tasks

skjnldsv requested changes Feb 20, 2017

View reviewed changes

Css guidelines compliance

ccb0cb2

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

raimund-schluessler changed the title ~~Fit to styleguide, closes #49~~ [WIP] Fit to styleguide, closes #49 Feb 20, 2017

raimund-schluessler closed this Feb 20, 2017

skjnldsv deleted the FixPopover branch February 20, 2017 22:50

[WIP] Fit to styleguide, closes #49 #51

[WIP] Fit to styleguide, closes #49 #51

Uh oh!

Conversation

raimund-schluessler commented Feb 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raimund-schluessler commented Feb 19, 2017

Uh oh!

skjnldsv Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

raimund-schluessler Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

raimund-schluessler Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsv Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

raimund-schluessler Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsv Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsv Feb 20, 2017

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Feb 20, 2017

Uh oh!

skjnldsv commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Feb 20, 2017

Uh oh!

raimund-schluessler commented Feb 20, 2017

Uh oh!

skjnldsv commented Feb 20, 2017

Uh oh!

raimund-schluessler commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raimund-schluessler commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skjnldsv commented Feb 20, 2017

Uh oh!

raimund-schluessler commented Feb 20, 2017

Uh oh!

skjnldsv commented Feb 20, 2017

Uh oh!

raimund-schluessler commented Feb 20, 2017

Uh oh!

jancborchardt commented May 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

raimund-schluessler commented Feb 18, 2017 •

edited

Loading

raimund-schluessler Feb 20, 2017 •

edited

Loading

skjnldsv commented Feb 20, 2017 •

edited

Loading

raimund-schluessler commented Feb 20, 2017 •

edited

Loading

raimund-schluessler commented Feb 20, 2017 •

edited

Loading