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

Cleanup structure #9982

Merged
merged 11 commits into from
Jul 20, 2018
Merged

Cleanup structure #9982

merged 11 commits into from
Jul 20, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 25, 2018

Fix #4047

@nextcloud/designers

This is a much cleaner structure for our software

Done

  • Removed div#content-wrapper
  • Removed any absolute positionning of our content
    • Now the whole page is scrolled, not a div within an absolute div
    • Android header is now collapsing because the scrolling behaviour has been restored to its original state
  • Merged header and div#header to follow html5 guidelines

Deprecated and guidelines

  • Do not use #content-wrapper anymore
  • If your app is injecting itself by replacing the #content element, make sure to keep the #content id
  • If you use the app-content-list standard, make sure to hide the app-content-details div when the list is visible in mobile mode (full screen) and hide the app-content-list when the details is visible
  • This is app-content-details and NOT app-content-detail

Overall structure

<header>
    <div class="header-left">
        <!-- apps menu -->
    </div>
    <div class="header-right">
        <!-- search - contactsmenu - settingsmenu - ... -->
    </div>
</header>
<div id="content" class="app-YOURAPPID">
    <div id="app-navigation" class="">
        <div class="app-navigation-new">
            <!-- app 'new' button -->
        </div>
        <ul id="usergrouplist">
            <!-- app navigation -->
        </ul>
        <div id="app-settings">
            <!-- app settings -->
        </div>
    </div>
    <div id="app-content">
        <div id="app-navigation-toggle" class="icon-menu"></div>
        <!-- app-content-wrapper is optional, only use if app-content-list  -->
        <div id="app-content-wrapper">
            <div class="app-content-list">
                <!-- app list -->
            </div>
            <div class="app-content-details"></div>
            <!-- app content -->
        </div>
    </div>
    <div id="app-sidebar"></div>
</div>

@skjnldsv skjnldsv added enhancement design Design, UI, UX, etc. 3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix. labels Jun 25, 2018
@skjnldsv skjnldsv added this to the Nextcloud 14 milestone Jun 25, 2018
@skjnldsv skjnldsv self-assigned this Jun 25, 2018
@stefan-niedermann
Copy link
Member

May i ask why no nav-tag is used for #app-navigation or .header-left?

@skjnldsv
Copy link
Member Author

@stefan-niedermann We use a nav now in the app list

<nav role="navigation">

But not on the app navigation, which we should, you're completely right :)

@rullzer
Copy link
Member

rullzer commented Jun 26, 2018

conflicts and failing tests

@juliushaertl
Copy link
Member

juliushaertl commented Jun 26, 2018

While I like the new structure in general, scrolling the page instead of the app content causes a weird double scrollbar, when the sidebar is visible:
image

@skjnldsv
Copy link
Member Author

@juliushaertl Ah damn, it make sense.
The sidebar should be fixed though, I'll change this so the scrollbar is on the real content then!

@codecov
Copy link

codecov bot commented Jun 28, 2018

Codecov Report

Merging #9982 into master will decrease coverage by 2.44%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #9982      +/-   ##
============================================
- Coverage     31.87%   29.42%   -2.45%     
+ Complexity    26208    26007     -201     
============================================
  Files          1673     1595      -78     
  Lines         96874    88869    -8005     
  Branches       1290        0    -1290     
============================================
- Hits          30874    26152    -4722     
+ Misses        66000    62717    -3283
Impacted Files Coverage Δ Complexity Δ
core/templates/layout.public.php 0% <0%> (ø) 0 <0> (ø) ⬇️
core/templates/layout.user.php 0% <0%> (ø) 0 <0> (ø) ⬇️
apps/provisioning_api/lib/Controller/AUserData.php 60.25% <0%> (-0.51%) 15% <0%> (ø)
lib/private/Updater/VersionCheck.php 85.71% <0%> (-0.34%) 8% <0%> (-1%)
config/config.sample.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/dav/lib/Server.php 0% <0%> (ø) 22% <0%> (ø) ⬇️
apps/dav/appinfo/app.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/dav/lib/CalDAV/CalendarRoot.php 0% <0%> (ø) 1% <0%> (-3%) ⬇️
apps/dav/composer/composer/autoload_classmap.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
lib/private/Settings/Personal/PersonalInfo.php 0% <0%> (ø) 16% <0%> (-5%) ⬇️
... and 93 more

@MorrisJobke
Copy link
Member

MorrisJobke commented Jun 28, 2018

  • the sidebar moves below the content on smaller screens:
    bildschirmfoto 2018-06-28 um 16 52 03

@juliushaertl
Copy link
Member

juliushaertl commented Jun 29, 2018

  • The sidebar doesn't cause the main content to resize (file actions/edit date are hidden)

image

@skjnldsv
Copy link
Member Author

@juliushaertl hum, strange, I don't have this. What browser and did you cleaned the cache?

@juliushaertl
Copy link
Member

Ah ok, seems that was a caching issue. But #9982 (comment) still exists and the breadcrumbs are not sticky anymore in the files app.

Besides that I really like it. It's a lot cleaner than before 👍

@skjnldsv
Copy link
Member Author

@juliushaertl yep, I have this to fix!
Incoming! 🚢

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 29, 2018
@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 29, 2018

  • Control sticky not working

@juliushaertl sticky is behaving strangely, do you know more about it than I do? :)

janLo added a commit to janLo/nextcloud-drawio that referenced this pull request Sep 7, 2018
The app content must live in #app-content, not #app anymore.
See nextcloud/server#9982

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
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 design Design, UI, UX, etc. enhancement papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants