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

feat(NcAppContent): add mobileLayout prop #6364

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

devlinjunker
Copy link

@devlinjunker devlinjunker commented Jan 11, 2025

☑️ 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

  • add mobileLayout property
  • add horizontal-split option for mobileLayout property

🏁 Checklist

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

Signed-off-by: Devlin Junker <[email protected]>
Signed-off-by: Devlin Junker <[email protected]>
@devlinjunker devlinjunker changed the title Add mobileLayout prop to NcAppContent feat(NcAppContent): Add mobileLayout prop to NcAppContent Jan 14, 2025
@devlinjunker
Copy link
Author

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

@ShGKme ShGKme added enhancement New feature or request 3. to review Waiting for reviews feature: app-content Related to the app-content component labels Jan 24, 2025
@ShGKme ShGKme added this to the 8.23.0 milestone Jan 24, 2025
Copy link

codecov bot commented Jan 24, 2025

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 ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Thanks, can be useful.

Please, fix linting issues like trailing spaces and commas.

src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
src/components/NcAppContent/NcAppContent.vue Outdated Show resolved Hide resolved
Comment on lines 415 to 420
&:not(.splitpanes--horizontal-mobile) {
.splitpanes__pane-list{
@media only screen and (width < $breakpoint-mobile) {
display: none;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's control it in JS instead as it was.

Copy link
Author

Choose a reason for hiding this comment

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

it was controlled with css before on lines 411-413 of the old file:

.splitpanes__pane {
  ...
  &-list {
    ...
    @media only screen and (width < $breakpoint-mobile) {
      display: none;
    }
  }
}

I could change it to javascript though if you think that is clearer

@ShGKme ShGKme requested a review from GretaD January 24, 2025 20:15
@ShGKme ShGKme changed the title feat(NcAppContent): Add mobileLayout prop to NcAppContent feat(NcAppContent): add mobileLayout prop Jan 24, 2025
@devlinjunker
Copy link
Author

Thanks for taking a look! Will do!

@GretaD
Copy link
Contributor

GretaD commented Jan 27, 2025

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.

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

I'd be fine if this is an option on desktop indeed, as Greta mentioned. Just like it's done in Mail already.

But this doesn't seem to be a common pattern on mobile? There's too little space to have 2 different scrolling areas and it will lead to confusion and e.g. accidentally messing with the split.

@devlinjunker
Copy link
Author

@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.

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Jan 28, 2025

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

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 enhancement New feature or request feature: app-content Related to the app-content component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants