Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Update gui.py #218

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

Open
saddy001 wants to merge 12 commits into rawpython:master
base: master
Choose a base branch
Loading
from saddy001:master
Open

Update gui.py #218

saddy001 wants to merge 12 commits into rawpython:master from saddy001:master

Conversation

@saddy001
Copy link

@saddy001 saddy001 commented Apr 17, 2018

allow treeopen keyword for TreeItem

saddy001 added 4 commits April 17, 2018 08:01
allow treeopen keyword for TreeItem
add text_align keyword to TableEditableItem
add text_align keyword to TableEditableItem
remi/gui.py Outdated
self.set_text(text)
self.treeopen = False
self.attributes['treeopen'] = 'false'
treeopen = kwargs.get('treeopen', False)
Copy link
Collaborator

@dddomodossola dddomodossola Apr 19, 2018

Choose a reason for hiding this comment

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

It could be ok to have a treeopen parameter, but it's better to have it as an explicit optional parameter.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. At the moment I don't know how to make a pull request after committing, so here's the link:
saddy001@3c5cb94#diff-4be7e258419023f7dad3cc9f659df8b9


@decorate_constructor_parameter_types([str])
def __init__(self, text='', text_align='left', **kwargs):
def __init__(self, text='', text_style={}, **kwargs):
Copy link
Collaborator

@dddomodossola dddomodossola Apr 19, 2018

Choose a reason for hiding this comment

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

A more proficient way to do styling would be to use css stylesheet. Otherwise we should add a style parameter for each widget in a container, like TableItem in Tables, ListItem in ListView and so on... Doesn't it?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know how to use separate stylesheets, because at the moment, there is no documentation about it ;-)
If it means that the whole original stylesheet has to be copied, I don't think it's a good idea...

Copy link
Collaborator

@saddy001 I really like the changes you made. I will review and test this, than I will merge. Thank you for this contribution.


@decorate_constructor_parameter_types([str])
def __init__(self, text='', **kwargs):
def __init__(self, text='', text_style={}, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

mutable functions args (dict in signature) are not a good idea

dddomodossola reacted with thumbs up emoji
Copy link
Author

@saddy001 saddy001 Sep 3, 2018

Choose a reason for hiding this comment

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

Yep. If you have your hands already on this, could you change it to None and assign
text_style = text_style or {}
?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nzjrs nzjrs nzjrs left review comments

@dddomodossola dddomodossola dddomodossola left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /