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

General layout improvement: Top level navigation + structure #6047

Draft
wants to merge 132 commits into
base: edge
Choose a base branch
from

Conversation

math-GH
Copy link
Contributor

@math-GH math-GH commented Jan 18, 2024

The first draft was presented in #5966. This is now the implementation.

close #4224

This PR is still in draft but I am happy about each comment or feedback

  • Roughly fine with Origine theme (wide screen)
  • Roughly fine with Origine theme (small screen/mobile devices)
  • introduce a new break point 1200px
  • Fine with each theme
    • Origine
    • Origine compact
    • Alternative dark
    • Dark/Dark Pink
    • Ansum/Mapco
    • Flat design
    • Nord
    • Pafat
    • Swage
  • Install routine page
  • views
    • normal view
    • global view
    • reading view
  • Anonym. reading
  • login page
  • about page as guest
  • TOS page as guest
  • check extensions
    • Colorful List
    • Custom CSS
    • Custom JS
    • Image Proxy (not relevant, but works fine)
    • Quick Collapse
    • ReadingTime
    • Share By Email <== buggy
    • Sticky Feeds (not necessary anymore)
    • Title-Wrap
    • ShowFeedID
    • ThreePanesView <== buggy
    • any other important extension, that needs to be checked?
  • navigation buttons
    • invisible buttons
    • visible buttons
    • left button
    • right button
    • up button
  • options for less buttons in the header
  • Update screenshots of the themes

known issues

  • Tested the shortcuts for switching the views
  • offset of mark read while scrolling
  • extensions
  • ....... please leave feedback and your testing results here in the comment sections ....

(more screenshots + description will follow)

Out of scope

(not so important, discussion needed, PR could follow):

  • [ ]reorganize the navigation side bar (aside_config.phtml, aside_subscription.phtml), following the new dropdown menu structure

@math-GH math-GH added UI 🎨 User Interfaces UX User experience labels Jan 18, 2024
@math-GH math-GH added this to the 1.24.0 milestone Jan 18, 2024
@math-GH math-GH marked this pull request as draft January 18, 2024 12:02
@Alkarex
Copy link
Member

Alkarex commented Jan 18, 2024

I like the fixed bar at the top 👍🏻

I am more neutral about the left bar. But it is nice with an update 🙂

The improvements of the mobile view are what I am most looking forward to.

A couple of notes, which you probably already have spotted:

  • The "plus" button (to add feeds) leads to a dead link
  • With the option "auto mark as read during scroll", the vertical cut before which articles should be marked as read should be adjusted a bit (at the moment, it is about ~2 articles off).

@Alkarex
Copy link
Member

Alkarex commented Mar 8, 2024

The complexity with the flush is that it depends on some buffering / size of content, at more than one place in the pipeline all the way to the screen of the user.
You can reproduce it by adding a sleep(10); for instance around:

$view->entries = FreshRSS_index_Controller::listEntriesByContext();

Comment on lines 87 to 110
$keepAllURLParameters = true;
$href = '';
if (!in_array(Minz_Request::actionName(), ['normal', 'global', 'reader'])) {
$keepAllURLParameters = false;

}

/** @var FreshRSS_ReadingMode $mode */
foreach ($readingModes as $mode) {
if ($keepAllURLParameters) {
// keep all URL parameters if the current page is a view
$href = Minz_Url::display($mode->getUrlParams());
} else {
echo FreshRSS_Context::systemConf()->logo_html;
$params = $mode->getUrlParams();
$href = Minz_Url::display(['c' => $params['c'], 'a' => $params['a']]);
}
?>

<a class="<?= $mode->getId() ?> btn <?php if ($mode->isActive()) { echo 'active'; } ?>" title="<?=
$mode->getTitle() ?>" href="<?= $href ?>">
<?= $mode->getName() ?>
</a>
<?php
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not the smartest solution to solve #6162 but it works so far.

@math-GH
Copy link
Contributor Author

math-GH commented Mar 10, 2024

Here are three regressions from some quick tests:

  1. Keyboard navigation: when landing on FreshRSS homepage without doing anything, I used to be able to click on the keyboard ↓ (down arrow) to scroll down the articles. That does not seem to work anymore in the current PR, as it first requires clicking somewhere in the article section for the scroll to work.

  2. Closing user labels: in this PR, closing the drop-down menu requires clicking again precisely on the menu link, instead of being able to click anywhere

@Alkarex This feedback was very valuable. It gave me the kick to finde a better solution as display:grid for the main layout. Now a lot of issues and workarounds are fixed (hopefully). The handling should be as it was before :) Please check it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI 🎨 User Interfaces UX User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Reading View: Feed menu in side bar: does not work
4 participants