Improved UI for Reading List Bar and Table of Contents #1256
Conversation
|
@heropj Thank you for your PR. I have rebased it on latest |
There was a problem hiding this comment.
@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.
src/tableofcontentbar.ui
Outdated
| @@ -7,7 +7,7 @@ | |||
| <x>0</x> | |||
| <y>0</y> | |||
| <width>400</width> | |||
| <height>300</height> | |||
| <height>300</height> | |||
There was a problem hiding this comment.
The changes in this file are unnecessary.
src/tableofcontentbar.cpp
Outdated
| @@ -94,7 +92,7 @@ void TableOfContentBar::setupTree(const QJsonObject& headers) | |||
| if (headerUrl != currentUrl) | |||
| return; | |||
|
|
|||
| m_url = headerUrl; | |||
| m_url = headerUrl; | |||
There was a problem hiding this comment.
Please remove trailing space.
src/tableofcontentbar.cpp
Outdated
| @@ -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)); | |||
There was a problem hiding this comment.
Do not leave commented out code without explanation.
resources/css/style.css
Outdated
| #readinglistbar QLabel { | ||
| /* General Style Improvements */ | ||
| #readinglistbar, #tableofcontentbar { | ||
| /* background-color: #F9F9F9; */ |
There was a problem hiding this comment.
Please do not leave commented out code without reasoning
|
@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. |
|
@heropj There are still 46 files changed. Please only leave those which are relevant. |
|
@ShaopengLin Got it..I have removed the commits which caused those unnecessary changes and have made a fresh commit with relevant changes only. |
There was a problem hiding this comment.
@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.
|
@ShaopengLin Thank you for the feedback, I will undo changes in TOC and make RL ui similar to it. |
fix #1247
Changes: