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

bugfix: ugly white border (gtk 3.20>) #128

Merged
merged 1 commit into from Jun 1, 2017
Merged

bugfix: ugly white border (gtk 3.20>) #128

merged 1 commit into from Jun 1, 2017

Conversation

ghost
Copy link

@ghost ghost commented May 31, 2017

No description provided.

Copy link
Contributor

@leigh123linux leigh123linux left a comment

Choose a reason for hiding this comment

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

You need to add conditionals so that gtk-3.20< isn't affected by this change

@JosephMcc
Copy link
Contributor

You need to add conditionals so that gtk-3.20< isn't affected by this change

Yes you do. As you can tell I've messed with that before. I'd rather it not affect gtk < 3.20.

@leigh123linux in Fedora you might want to tweak your theme a bit once this is merged. I did this in Mint-Y: linuxmint/mint-y-theme@735fd68#diff-68426d2ebfa0f55850ee1027cde252bfR841

It brings back the border between the bottom bars and the notebook.

@ghost
Copy link
Author

ghost commented May 31, 2017

ok I have added a gtk version check before I hide the border. ;)

@JosephMcc
Copy link
Contributor

JosephMcc commented May 31, 2017

Sorry to keep nitpicking you on such a small thing but I'd prefer if you follow our typical conventions for this.

    gtk_notebook_set_scrollable (GTK_NOTEBOOK (notebook), TRUE);

#if GTK_CHECK_VERSION (3, 20, 0)
    gtk_notebook_set_show_border (GTK_NOTEBOOK (notebook), FALSE);
#endif

    gtk_notebook_set_show_tabs (GTK_NOTEBOOK (notebook), FALSE);

This makes them easier to find and clean up later on.

When formatting your code using if() statements and such please follow conventions used throughout the rest of the code. So something like

if (some_condition)
{
    // some stuff
}

I'd appreciate if you you update the PR and squash it down to a single commit. Not sure we need three commits to change a single line :)

@JosephMcc
Copy link
Contributor

Thanks.

@ghost
Copy link
Author

ghost commented May 31, 2017

Done! 🍔 :)

@itzexor
Copy link
Contributor

itzexor commented May 31, 2017

I tested this and it fixes the issue on gtk 3.22.15. Checked with a few gtk themes and xed themes and didn't see any issues. 👍

@clefebvre clefebvre merged commit f004e93 into linuxmint:master Jun 1, 2017
@leigh123linux
Copy link
Contributor

@JosephMcc Thanks for the pointer, does this commit look ok?

horst3180/arc-theme#790

@JosephMcc
Copy link
Contributor

@leigh123linux I don't think you can use the XedFileBrowserWidget class name. I haven't worked much yet with theming beyond 3.18 but I don't think the use of class names is supported any longer. You would have to test on your end but I think .xed-window .small-button would work in this case. So yo could add the .small-button part within your .xed-window block in the sass file.

@leigh123linux
Copy link
Contributor

leigh123linux commented Jun 1, 2017

@JosephMcc Thanks, does that look better?

I don't see any additional warnings with either.

@JosephMcc
Copy link
Contributor

As far as I understand it, that should work. You should be able to tell. It makes the navigation buttons in the sidebar file browser widget a good bit smaller. They were quite large originally in Mint-Y without that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants