Improved UI for Reading List Bar and Table of Contents #1256

heropj · 2024-11-23T14:40:27Z

Changes:

Standardized UI for Reading List Bar and Table of Contents.

kelson42 · 2024-11-26T13:42:18Z

@heropj Thank you for your PR. I have rebased it on latest main branch and we will review it.

ShaopengLin

@heropj Please first remove the superfluous changes before we move to a formal review. The changes to some files like the icons may be due to your formatter? It is important to keep commit footprint at a minimum, as it makes it difficult for reviewers to navigate your changes.

ShaopengLin · 2024-11-26T16:39:01Z

src/tableofcontentbar.ui

@@ -7,7 +7,7 @@
    <x>0</x>
    <y>0</y>
    <width>400</width>
-    <height>300</height>
+    <height>300</height> 


The changes in this file are unnecessary.

ShaopengLin · 2024-11-26T16:39:30Z

src/tableofcontentbar.cpp

@@ -94,7 +92,7 @@ void TableOfContentBar::setupTree(const QJsonObject& headers)
    if (headerUrl != currentUrl)
        return;

-    m_url = headerUrl;
+    m_url = headerUrl; 


Please remove trailing space.

ShaopengLin · 2024-11-26T16:40:00Z

src/tableofcontentbar.cpp

@@ -9,10 +9,8 @@ TableOfContentBar::TableOfContentBar(QWidget *parent) :
    ui(new Ui::tableofcontentbar)
 {
    ui->setupUi(this);
-    ui->titleLabel->setFont(QFont("Selawik", 18, QFont::Weight::Medium));
+    // ui->titleLabel->setFont(QFont("Selawik", 18 , QFont::Weight::Medium));


Do not leave commented out code without explanation.

ShaopengLin · 2024-11-26T16:40:51Z

resources/css/style.css

-#readinglistbar QLabel {
+/* General Style Improvements */
+#readinglistbar, #tableofcontentbar {
+    /* background-color: #F9F9F9;  */


Please do not leave commented out code without reasoning

heropj · 2024-11-27T17:54:08Z

@ShaopengLin Sorry for the inconvenience caused by the superfluous changes. In my latest commit I have ensured no such changes are present. I’ve addressed other changes also as per your suggestion.

ShaopengLin · 2024-11-27T18:21:14Z

@heropj There are still 46 files changed. Please only leave those which are relevant.

heropj · 2024-11-27T19:11:08Z

@ShaopengLin Got it..I have removed the commits which caused those unnecessary changes and have made a fresh commit with relevant changes only.

ShaopengLin

@heropj The issue wants you to match ReadingList to the behaviour and look of the TOC, and we should not change the TOC. There are reasons for the design of TOC to be like this so please do not change it and focus on the 3 points I mentioned in the Issue.

SInce there are only a few UI changes, I will first focus on the UI. Once that's up-to-par (Kelson might also grill you similar to how he did to me :) later ), I can review the coding part.

heropj · 2024-11-30T08:56:48Z

@ShaopengLin Thank you for the feedback, I will undo changes in TOC and make RL ui similar to it.

heropj · 2024-12-01T07:28:39Z

@ShaopengLin

ShaopengLin · 2024-12-02T19:46:52Z

src/mainwindow.cpp

@@ -197,6 +197,7 @@ void MainWindow::tableOfContentToggled(bool state)
    if (state) {
        checkActionNoSignal(KiwixApp::ToggleReadingListAction, false);
        mp_ui->sideBar->setCurrentWidget(mp_ui->tableofcontentbar);
+        mp_ui->sideBar->setStyleSheet("background-color: white;");


This is a bug. If the state has not changed, the stylesheet won't change. Like image below.

You should be able to change the background color like TOC without specifically setting the stylesheet on sideBar, try to investigate the CSS changes before doing ad-hoc changes like this.

ShaopengLin · 2024-12-02T19:47:49Z

src/readinglistbar.cpp

@@ -15,6 +15,16 @@ ReadingListBar::ReadingListBar(QWidget *parent) :
    ui(new Ui::readinglistbar)
 {
    ui->setupUi(this);
+
+    //to remove gap between RL components
+    if (layout()) {


Why is this check necessary? I don't see setLayout() calls later so this state won't change.

ShaopengLin · 2024-12-03T18:26:22Z

@heropj The change LGTM, but do change the commit message to be more generalized and meaningful.

@veloman-yunkan @kelson42 The UI and code look ok to me. Please have a round of review for any further suggestions.

ShaopengLin

Please fix the following

ShaopengLin · 2024-12-05T22:34:40Z

src/mainwindow.cpp

@@ -197,7 +197,6 @@ void MainWindow::tableOfContentToggled(bool state)
    if (state) {
        checkActionNoSignal(KiwixApp::ToggleReadingListAction, false);
        mp_ui->sideBar->setCurrentWidget(mp_ui->tableofcontentbar);
-        mp_ui->sideBar->setStyleSheet("background-color: white;");


This change shouldn't be there in the first place. Do not let things like this appear in the commit history.

ShaopengLin · 2024-12-05T22:34:59Z

src/readinglistbar.cpp

    ui(new Ui::readinglistbar)
 {
    ui->setupUi(this);
-
-    //to remove gap between RL components
-    if (layout()) {


ShaopengLin · 2024-12-05T22:36:31Z

@heropj We want the commit message to be both concise and informative.

final fix of RL

is not informative on the changes you did.

The second commit is way too long and thus not concise.

heropj · 2024-12-06T19:33:07Z

@ShaopengLin done😢😅

kelson42 · 2024-12-07T11:35:58Z

@heropj @ShaopengLin Thank you very much. I will review this PR from a user perspective this week-end and I hope @veloman-yunkan can do the same from the code perspective.

veloman-yunkan · 2024-12-08T09:33:00Z

src/readinglistbar.cpp

    ui(new Ui::readinglistbar)
 {
    ui->setupUi(this);
+    ui->titleLabel->setFont(QFont("Selawik", 18, QFont::Weight::Medium));


Why is the font set in C++ code? Can't it be controlled via CSS?

Yes it can be set via CSS but TOC also had font set in cpp code in tableofcontentbar.cpp... so to maintain code consistency i added this line here in readinglistbar.cpp.

@ShaopengLin Any reason the fonts in TOC sidebar are controlled from C++ rather than CSS?

If possible I am ok with doing this in CSS. Its just my personal preference to control font in Qt, as I remember some widgets doesn't support CSS fonts.

Hello.. I just checked it's working via CSS, so should i do it in CSS??

Yes, please

veloman-yunkan · 2024-12-08T09:44:30Z

src/readinglistbar.cpp

@@ -37,7 +40,6 @@ ReadingListBar::ReadingListBar(QWidget *parent) :
    auto importAction = app->getAction(KiwixApp::ImportReadingListAction);
    connect(exportAction, &QAction::triggered, this, &ReadingListBar::onExport);
    connect(importAction, &QAction::triggered, this, &ReadingListBar::onImport);
-    ui->label->setText(gt("reading-list-title"));


So this was replaced with ui->titleLabel->setText(gt("reading-list")); (and moved to the beginning of the constructor body). The new name of the label data member makes more sense, however it's not clear why the i18n text string id was changed from "reading-list-title" to "reading-list".

But checking resources/i18n/qqq.json and resources/i18n/en.json raises another question - do we really need both those strings? One of them is used as the tooltip for the reading list button and the other as the heading of the reading list bar. In English translation file the difference between them is only in capitalization, while their descriptions in qqq.json are identical. We should either drop one of them, or describe them differently in qqq.json.

@ShaopengLin @kelson42

Hello! thank you for reviewing.. the reason i changed it from "reading-list-title" to "reading-list" is to maintain consistency with code present in tableofcontentbar.cpp😅 and since it worked i didn't got into checking the difference it might cause...but now that you have investigated about these strings, let me know how we should proceed further...drop one of them or change their description.

@kelson42 Will you please provide your feedback to my comment above?

@veloman-yunkan Nomstrong opinion, but if I have to choose I would say that having two strings seems wrong.

@heropj Then let's get rid of the "reading-list-title" and use "reading-list" instead. But I think that its capitalization must be changed to title case.

@veloman-yunkan Do i need to remove "reading-list-title" from all translation files?? or just from qqq.json and en.json??

Only qqq.json and en.json must be updated.

kelson42

LGTM from a user perspective

veloman-yunkan

The final result looks good to me, but the commit history falls a little short of what I expect from a good code evolution history. The main problem is that the second commit combines two unrelated changes and rather reflects the review process than a logical and clean series of steps of arriving at the agreed state. Please rewrite the commit history of this PR as follows:

Dropping of the "reading-list-title" entry from translated strings (C++ code using the removed entry must be updated accordingly, each commit must be self-consistent).
Restyling of the Reading List sidebar (rest of the changes).

kelson42 requested a review from veloman-yunkan November 23, 2024 17:31

kelson42 force-pushed the ui/tr branch from c64f616 to aa2f94b Compare November 26, 2024 13:41

kelson42 requested review from kelson42 and ShaopengLin November 26, 2024 13:42

ShaopengLin requested changes Nov 26, 2024

View reviewed changes

heropj force-pushed the ui/tr branch from dd4f1d6 to 2632af5 Compare November 27, 2024 19:03

ShaopengLin reviewed Nov 29, 2024

View reviewed changes

heropj force-pushed the ui/tr branch 2 times, most recently from 02957cf to efc5e9e Compare December 1, 2024 07:24

ShaopengLin requested changes Dec 2, 2024

View reviewed changes

heropj force-pushed the ui/tr branch from b2bdc41 to 1ca027b Compare December 3, 2024 18:25

heropj force-pushed the ui/tr branch from 1ca027b to 3fcc995 Compare December 3, 2024 18:47

ShaopengLin approved these changes Dec 5, 2024

View reviewed changes

ShaopengLin requested changes Dec 5, 2024

View reviewed changes

heropj force-pushed the ui/tr branch from 3fcc995 to e0556b4 Compare December 6, 2024 19:28

ShaopengLin approved these changes Dec 6, 2024

View reviewed changes

veloman-yunkan reviewed Dec 8, 2024

View reviewed changes

kelson42 force-pushed the ui/tr branch from 096a325 to 922cf46 Compare December 10, 2024 19:32

kelson42 approved these changes Dec 10, 2024

View reviewed changes

veloman-yunkan reviewed Dec 11, 2024

View reviewed changes

heropj force-pushed the ui/tr branch from 922cf46 to 5230494 Compare December 12, 2024 17:38

veloman-yunkan approved these changes Dec 12, 2024

View reviewed changes

heropj added 2 commits December 12, 2024 21:26

Restyling of the Reading List sidebar

773fb41

Dropping of the reading-list-title entry from translated strings

19707e1

kelson42 force-pushed the ui/tr branch from 6cb8328 to 19707e1 Compare December 12, 2024 20:26

kelson42 merged commit 74111c8 into kiwix:main Dec 12, 2024
6 checks passed

Improved UI for Reading List Bar and Table of Contents #1256

Improved UI for Reading List Bar and Table of Contents #1256

Conversation

heropj commented Nov 23, 2024

kelson42 commented Nov 26, 2024

ShaopengLin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heropj commented Nov 27, 2024

ShaopengLin commented Nov 27, 2024 • edited Loading

heropj commented Nov 27, 2024

ShaopengLin left a comment • edited Loading

Choose a reason for hiding this comment

heropj commented Nov 30, 2024

heropj commented Dec 1, 2024

ShaopengLin Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaopengLin commented Dec 3, 2024 • edited Loading

ShaopengLin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaopengLin commented Dec 5, 2024

heropj commented Dec 6, 2024

kelson42 commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelson42 left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

ShaopengLin left a comment •

edited

Loading

ShaopengLin commented Nov 27, 2024 •

edited

Loading

ShaopengLin left a comment •

edited

Loading

ShaopengLin Dec 2, 2024 •

edited

Loading

ShaopengLin commented Dec 3, 2024 •

edited

Loading

kelson42 Dec 10, 2024 •

edited

Loading