-
Notifications
You must be signed in to change notification settings - Fork 92
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
[GUI][CSS] Refactored Dark Mode #933
base: master
Are you sure you want to change the base?
[GUI][CSS] Refactored Dark Mode #933
Conversation
….git into css_refactor_local
Wallet is ready for testing. I expect there to be some potential styling/formatting changes requested but otherwise the first working version of dark theme is here (FAQ tab pop-up has some issues) |
Other anomaly in Dark Mode (only experienced at wallet creation or restore). |
I will definitely be squashing most if not all of the commits. This is a wallet-wide change that touches just about every gui page. |
I'd be interested in having another look at this one some time, to not waste the work, and because I have appreciated the results of this Dark Mode style. So, when you're ready, just say the word. |
} | ||
return QString(); | ||
|
||
if(fileDark.open(QFile::ReadOnly) && isDarkModeSet){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please use consistent style for spacing and indentation here. I prefer space after if
and before {
and 4-space indents (Github seems to round tabs to 4 or 8 spaces depending on space before them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will likely not be getting to this PR anytime soon but commenting as noted.
@@ -550,6 +550,7 @@ void OverviewPage::showEvent(QShowEvent *event){ | |||
if (fHide != filter->orphansHidden()) | |||
hideOrphans(fHide); | |||
|
|||
/* | |||
QGraphicsOpacityEffect *eff = new QGraphicsOpacityEffect(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and several other (animation?) effects have been remarked out in this PR. Was this a debugging change, or is there a reason to remove these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i remember correctly, this was because the animation is clashing with the styling (causes flicker or just looks bad and is unnecessary). If you add it back in, would just need an eyeball check as it is all cosmetic changes.
@@ -49,7 +49,7 @@ SplashScreenVeil::SplashScreenVeil(interfaces::Node& node, Qt::WindowFlags f, co | |||
this->setStyleSheet(GUIUtil::loadStyleSheet()); | |||
|
|||
this->resize(800, 514); | |||
ui->frame->setStyleSheet("#frame{border-image: url(\":/icons/splash_background\") 0 0 0 0 stretch stretch;padding: 0;margin: 0;}"); | |||
//ui->frame->setStyleSheet("#frame{border-image: url(\":/icons/splash_background\") 0 0 0 0 stretch stretch;padding: 0;margin: 0;}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we are setting the stylesheet here and the whole purpose is the stylings are now in css instead of being handled in the code. It should have a matching styling in the css files.
|
||
QPoint pos = mainWindow->getGUI()->mapFromGlobal(QCursor::pos()); | ||
|
||
if(pos.x()+menu->width()>mainWindow->getGUI()->width()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: spacing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will likely not be getting to this PR anytime soon but commenting as noted.
const QString constType = type; | ||
if(!this->menu) this->menu = new AddressesMenu(constType , updatedIndex, this, this->mainWindow, this->model); | ||
if(!this->menu) this->menu = new AddressesMenu(constType , updatedIndex, mainWindow->getGUI(), this->mainWindow, this->model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(!this->menu) this->menu = new AddressesMenu(constType , updatedIndex, mainWindow->getGUI(), this->mainWindow, this->model); | |
if (!this->menu) | |
this->menu = new AddressesMenu(constType, updatedIndex, mainWindow->getGUI(), this->mainWindow, this->model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will likely not be getting to this PR anytime soon but commenting as noted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK d28da77
We can address nits in a followup; I was going to do them myself but it seems inappropriate for me to then review my own commit.
I'll have to retest this PR. I'd like to see it released, and the other @Rock-N-Troll PRs need to be considered, including #926. |
New Text color change (will require icon to be copied and recolored):
Previous Text color unchanged: