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

add-shortcut-printer #4848

Merged
merged 4 commits into from Oct 8, 2019

Conversation

@estebanlm
Copy link
Member

estebanlm commented Oct 7, 2019

adding a generic shortcut printer (needed to properly print menus for different platforms and backends).

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 7, 2019

esteban this is cool. Now having some tests would be nice too.
Because one day we will break it.

@estebanlm

This comment has been minimized.

Copy link
Member Author

estebanlm commented Oct 7, 2019

not finished :)

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 7, 2019

:) but going to bed now :)

@estebanlm

This comment has been minimized.

Copy link
Member Author

estebanlm commented Oct 8, 2019

now is good to integration.

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

You see
ToggleMenuItemShortcut >> color [

^ color ifNil: [ Smalltalk ui theme ghostTextColor ]

]

I think that this guy should either be a subclass of displayableObject.
We should stop adding Smalltalk ui theme and other like that in the code.

@estebanlm

This comment has been minimized.

Copy link
Member Author

estebanlm commented Oct 8, 2019

you ask it, you have it

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

Ok cool now I do not understand why I still see

ToggleMenuItemShortcut >> color [

^ color ifNil: [ Smalltalk ui theme ghostTextColor ]

]
It should probably be something like [self theme ghostTextColor] no?

@estebanlm

This comment has been minimized.

Copy link
Member Author

estebanlm commented Oct 8, 2019

is just that my network was failing and the push didn't arrive until a bit later.

@Ducasse

This comment has been minimized.

Copy link
Member

Ducasse commented Oct 8, 2019

oki

@Ducasse Ducasse merged commit 47961e1 into Pharo8.0 Oct 8, 2019
1 of 4 checks passed
1 of 4 checks passed
continuous-integration/benchmarks The benchmarks show regressions.
Details
continuous-integration/jenkins/pr-merge This commit has test failures
Details
probot/minimum-reviews Pending review approvals
WIP Ready for review
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.