Hightlight properly library items #1062
Conversation
|
@MeerIsmailAli Which issue does it fix? Please open an issue explain the nature of the bug/enhancement. |
kelson42
force-pushed
the
hover_ui_changes
branch
from
060144a to
1a7831c
Compare
|
@MeerIsmailAli Thank you very much for your PR and sorry for the late feedback my side. We will review your PR from both functional and code perspective. I have also rebased your PR on latest |
|
@MeerIsmailAli From a functional point of vue, it works fine, but can we secure as well the line between each item goes from the very left to the very right, like in the mockup? |
|
is this sufficient @kelson42 ? |
kelson42
force-pushed
the
hover_ui_changes
branch
from
5897f5d to
2afc97c
Compare
|
@MeerIsmailAli Thank you, but why the separator does not go on the very left below the arrows? |
|
@MeerIsmailAli Hi, do you still plan to cpmplete the last small step so we can merge? |
kelson42
force-pushed
the
hover_ui_changes
branch
from
2afc97c to
7617728
Compare
|
@kelson42 ok let me look into it |
There was a problem hiding this comment.
@MeerIsmailAli Thank you for your last commits, now it looks good from grey background perspective and separator... but you have introduced a regression. The description font is now partly in grey, it should stay the same color like before. See my screenshot showing the problem:
The git revision log is pretty messy because of multiple commits and also merge of main branch in your branch. Please put all your changes in one commit... on the top of current HEAD of main branch. Then we could start with technical code review.
|
@MeerIsmailAli Any update? We are almost there! |
|
sorry for the confusion tried to consolidate all changes in a new pr, but removed it now , made the recent commits to handle the recent regression. |
|
@MeerIsmailAli Can you please rebase on the HEAD of main branch please? |
|
@kelson42 my branch has been rebased with the main branch, im not sure what exactly im missing. Let me know if theres any changes needed. |
|
@MeerIsmailAli You don't have rebased on |
src/contentmanagerdelegate.cpp
Outdated
| if (option.state & QStyle::State_MouseOver) { | ||
| baseButton->setStyleSheet("background-color: #eaecf0;" | ||
| "border: 0;" | ||
| "font-weight: bold;" | ||
| "font-family: Selawik;" | ||
| "border-bottom: 1px solid #cccccc;" | ||
| "color: blue;" | ||
| "margin: 0;"); | ||
| baseButton->style()->drawControl( QStyle::CE_PushButton, &button, painter, baseButton.data()); | ||
| }else{ | ||
| baseButton->setStyleSheet("background-color: white;" | ||
| "border: 0;" | ||
| "font-weight: bold;" | ||
| "font-family: Selawik;" | ||
| "border-bottom: 1px solid #cccccc;" | ||
| "color: blue;" | ||
| "margin: 0;"); | ||
| baseButton->style()->drawControl( QStyle::CE_PushButton, &button, painter, baseButton.data()); | ||
| } |
There was a problem hiding this comment.
Too much code duplication between the two branches. At least the drawControl() line can be moved out.
Then the only difference between the stylesheet strings is the value of background colour. The question is if you had to explicitly specify the other parameters (using their implicitly default values in this context) just because it is impossible to separately set just the background colour.
In any case if the stylesheet string must be provided fully its common part should be separated into a constant.
There was a problem hiding this comment.
@veloman-yunkan thanks for your suggestions, implemented them in the latest commit.
|
@MeerIsmailAli Any update? We would really like to merge your PR. |
veloman-yunkan
force-pushed
the
hover_ui_changes
branch
from
8e7dfdc to
01776d7
Compare
|
I cleared the mess with the history of this PR. Then an attempt to rebase it on main failed since conflicting changes have been made in |
|
Superseeded by #1109 |
Changes in contentmanagerdelgate inside the paint for hovering
Fixes #999