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

Match header-padding with AppDetailsToggle-width #2693

Closed
wants to merge 1 commit into from

Conversation

ahstro
Copy link
Member

@ahstro ahstro commented Feb 26, 2020

Fixes problem with AppsDetailsToggle-button overlapping part of the message header on narrow screens.
unfixed
fixed

Signed-off-by: Anton Strömkvist <anton@stromkvist.com>
@welcome
Copy link

welcome bot commented Feb 26, 2020

Thanks for opening your first pull request in this repository! ✌️

@jancborchardt
Copy link
Member

@ahstro since the position change will make the subject not aligned with the body text anymore, could you instead fix this by removing the white background from the back arrow, or at least making it smaller? (The clickable area should stay the same, but the visual circle should be smaller.)

@ahstro
Copy link
Member Author

ahstro commented Feb 27, 2020

@jancborchardt Oh, should've seen that 🤦 Sure, I'm on it!

@ahstro
Copy link
Member Author

ahstro commented Feb 27, 2020

Okay, so CSS-issues are always the most fun 😅 Trying to make the background transparent reveals the icon hiding behind it.
bgtrans

Shrinking the width works visually, but secretly exposes some of the clickable area of the hidden button. (Blue dashed is toggle, green dotted is hidden)
shrank_000

I feel like whatever my uneducated solution would be, would end up breaking some other style somewhere, so I'll just.. fade away 👻 (Unless y'all want me to try to go wild, or have some pointers/ideas about a solution to take?) I guess one solution would be shrinking both buttons, using margins to keep them in place if we don't want them to move about? Just spitballing here 😇

@jancborchardt
Copy link
Member

@ahstro aah yes, that old problem. :)

@ChristophWurst is it not possible via Vue to make the display of the menu and back icons mutually exclusive?

@ChristophWurst
Copy link
Member

@ChristophWurst is it not possible via Vue to make the display of the menu and back icons mutually exclusive?

Wasn't there a ticket for this somewhere? I feel like this discussion has already happened and the css hack is the only viable solution. @Greta @skjnldsv maybe?

@szaimen
Copy link
Contributor

szaimen commented Feb 27, 2020

Shrinking the width works visually, but secretly exposes some of the clickable area of the hidden button. (Blue dashed is toggle, green dotted is hidden)
shrank_000

What about shrinking both buttons a bit? then the clickable area of the hidden button doesn't overlap...

@jancborchardt
Copy link
Member

The minimum clickable size needs to be 44*44px as per the design guidelines. :)

@skjnldsv
Copy link
Member

Wasn't there a ticket for this somewhere? I feel like this discussion has already happened and the css hack is the only viable solution. @Greta @skjnldsv maybe?

Yeah, the discussion was on the mail repo, but there might be a thread there too. @jancborchardt might now better

@szaimen
Copy link
Contributor

szaimen commented Feb 27, 2020

another idea: what about moving the title of the mail a bit to the right, then it doesn't overlap with the button... I mean it is more annoying (at least imo) seeing the cut off title then if the title doesn't align with the mail body below...

@szaimen
Copy link
Contributor

szaimen commented Feb 27, 2020

I would say the cleanest solution would be having a dedicated interaction bar for mobile on the top, that would be always visible like what are you doing it in the files app or text app.

I know it is a lot of space for just one back button but maybe you could also put the name of the app there if there is too much empty space.

The most important thing concerning that solution would be that it would solve all those annoying problems with overlapping and so on and could also work for all other apps.

Look at only one application from ios or android: most of them have interaction bars on the top.

@jancborchardt
Copy link
Member

@ChristophWurst what was the CSS hack though?


what about moving the title of the mail a bit to the right, then it doesn't overlap with the button

@szaimen this is what this pull request initially did, check my comment above for why it doesn’t look nice.

Look at only one application from ios or android: most of them have interaction bars on the top.

We already have our blue header bar with all the apps, and the browser controls there. We shouldn’t have yet another element.

@szaimen
Copy link
Contributor

szaimen commented Mar 24, 2020

We already have our blue header bar with all the apps, and the browser controls there. We shouldn’t have yet another element.

@jancborchardt Maybe I haven't described well enough what I mean. Please look at this fast mockup:
(You could use the available place to e.g. show the current imap-folder; which is not visible by default in mobile view; this new section would always be on top to make sure that you are able to control the app)
image

It is inspired by the files app where you have already a row for the app control elements:
image

BTW: you could use that now available space in the mails-view (if you click on a mail) to show the header of the mail... but that will be discussed after #2222 (comment) is merged.

@szaimen
Copy link
Contributor

szaimen commented Mar 24, 2020

This is how the same concept could apply to e.g. the contacts app:
(I am not saying that my solution is perfect but it solves so many problems that it should get considered.)
image

image

@jancborchardt
Copy link
Member

jancborchardt commented Mar 25, 2020

@szaimen there is a reason why we don’t show the current folder in the view – cause we lack space, and it’s not really necessary. Especially the last mockup shows that the "Contact name" will be duplicated by this new heading. We don’t want more interface, especially not vertically where space is limited when you also open the virtual keyboard.

And actually reading this again, we already have a solution with mockup for this at https://github.com/nextcloud/server/issues/16934#issuecomment-526664888 – it includes moving the "New message" button.

Since it’s mainly an issue with the Contacts and Mail apps, here’s another proposal:

  • We actually make space for the menu icon in the content area, moving the first content further down.
  • The "New message"/"New contact" button can then also go to the top of the list of messages/contacts instead of the top of the navigation.
  • This moves it more in line of how we do it in Files with the "+ New" button being in the main content area, not in the navigation. Especially for Contacts this then also creates Contacts where they will appear, directly on top of the list.
  • On mobile, this makes the action directly available without needing to open the navigation.
  • When scrolling, the menu and "new" elements could scroll out of view, to appear immediately whenever you scroll up a bit. Kind of like we do it on our website with the navigation.

In Contacts app cc @nextcloud/contacts
Contacts app menu

In Mail app cc @nextcloud/mail
Mail app menu

@jancborchardt
Copy link
Member

@ahstro do you want to modify the pull request to implement my design spec with the "New message" button moved to the top of the message list? :)

This would actually also solve the problem of the multiselect making the list jump when you are at the very top of the list #2273 cc @StCyr

@szaimen

This comment has been minimized.

@jancborchardt
Copy link
Member

So @skjnldsv mentioned it would be good to implement this in the Vue components at https://github.com/nextcloud/nextcloud-vue@skjnldsv do you have pointers for @ahstro or anyone who would like to dive into that? :)

@skjnldsv
Copy link
Member

So @skjnldsv mentioned it would be good to implement this in the Vue components at nextcloud/nextcloud-vue@skjnldsv do you have pointers for @ahstro or anyone who would like to dive into that? :)

Hey 👋
So We use the AppNavigation component alongside the AppContent.
We also have the AppContentList here https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/AppContentList/AppContentList.vue

I'm not fully aware of the specs, but if the goal is to migrate the New XXX button out of the Navigation always, then this needs to be done here https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/AppNavigationNew/AppNavigationNew.vue

@jancborchardt make sure the entire spec is super crystal clear here as this can be quite complicated I think (not saying it isn,t, as I did not read it all, just stating something we should be careful of :) )

I would list all the use cases and UX flows so that we're all on the same page 🚀

@szaimen

This comment has been minimized.

@jancborchardt

This comment has been minimized.

@ChristophWurst
Copy link
Member

I'm closing this one-liner PR due to the lack of activity. If someone wants to continue this please start a new PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants