nextcloud-vue icon indicating copy to clipboard operation
nextcloud-vue copied to clipboard

feat(NcAppContent): add `mobileLayout` prop

Open devlinjunker opened this issue 1 year ago • 6 comments

☑️ Resolves

  • Enables horizontal-split layout for mobile devices. This can be useful if you want to still see the list and details at the same time
  • Defaults to no-split still so no change for callers that don't pass in the mobileLayout property

🖼️ Screenshots

🏚️ Before 🏡 After
Jan-10-2025 17-52-17 Jan-10-2025 17-53-38

🚧 Tasks

  • [x] add mobileLayout property
  • [x] add horizontal-split option for mobileLayout property

🏁 Checklist

  • [X] ⛑️ Tests are included or are not applicable
  • [X] 📘 Component documentation has been extended, updated or is not applicable ~- [ ] 3️⃣ Backport to next requested with a Vue 3 upgrade~

devlinjunker avatar Jan 11 '25 01:01 devlinjunker

Any thoughts or feedback here? @ShGKme @raimund-schluessler

devlinjunker avatar Jan 24 '25 17:01 devlinjunker

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 42.50%. Comparing base (0d3715f) to head (9b6f558). Report is 146 commits behind head on master.

Files with missing lines Patch % Lines
src/components/NcAppContent/NcAppContent.vue 80.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6364      +/-   ##
==========================================
+ Coverage   42.26%   42.50%   +0.23%     
==========================================
  Files         155      156       +1     
  Lines        3996     4028      +32     
  Branches     1001     1038      +37     
==========================================
+ Hits         1689     1712      +23     
- Misses       2199     2200       +1     
- Partials      108      116       +8     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 24 '25 19:01 codecov[bot]

Thanks for taking a look! Will do!

devlinjunker avatar Jan 24 '25 20:01 devlinjunker

I think it looks very nice, but i remember when we discussed this feature, it was proposed to be shown only on big screens and not on mobile. So, looping @jancborchardt @nimishavijay @marcoambrosini to decide if this should go in.

GretaD avatar Jan 27 '25 12:01 GretaD

@jancborchardt @GretaD in an app like nextcloud/news it is useful to see both the list and chosen item at the same time on mobile so that you can navigate between the items and not have to do multiple clicks to select the next item to read.

accidentally messing with the split

If you test it out in the PR I linked above, it is very difficult to accidentally change the split height instead of scroll.

Since this new property defaults to no-split then it is up to the app development team to deliberately implement too and there is no behavior change unless they are interested in providing the horizontal mobile split to their users. I would think this change can be merged and just rarely implemented if you are worried about it not being very useful in other apps, but for news I think it will be very useful as right now the mobile behavior is not preferable IMO.

devlinjunker avatar Jan 27 '25 14:01 devlinjunker

I think that having 2 stacked scroll containers on a phone screen is bound to lead to bad UX consequences.

marcoambrosini avatar Jan 28 '25 10:01 marcoambrosini

See both comments with changes requested. If you think you will work on the changes needed, then please feel free to reopen, we are happy to move this forward then.

susnux avatar Aug 18 '25 20:08 susnux