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.

Sign up for GitHub

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

Jump to bottom

Feature/34 pipette #47

Merged
merged 17 commits into from
May 20, 2021
Merged

Feature/34 pipette #47

merged 17 commits into from
May 20, 2021

Conversation

maximilian-franz
Copy link
Contributor

Closes #34.

@maximilian-franz maximilian-franz self-assigned this May 14, 2021
@kpostnov
Copy link
Contributor

kpostnov commented May 15, 2021 •
edited
Loading

The implementation as it is looks good so far. However there are still some things that should be considered before this PR is to be merged:

  • The pipette tool icon doesn't match the actual tool yet.
  • M2Pipette should get a proper class comment just like the other M2MenuItem classes.
  • MorphicMonet >> chooseColor doesn't fit into the accessing category.
  • static doesn't contain any methods anymore wherefore it should be removed completely.

@ClFeSc ClFeSc force-pushed the feature/34-pipette branch from 6430952 to 86344c2 Compare May 16, 2021 16:57
@kpostnov kpostnov force-pushed the feature/34-pipette branch from 2892530 to db03dbe Compare May 18, 2021 22:26
maximilian-franz
maximilian-franz commented May 19, 2021
View reviewed changes
packages/SketchMorph2-Core.package/CursorWithAlpha.extension/class/fromIcon..st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2CircleBrush.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2ClearCanvas.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2Eraser.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2ExportMorph.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/menuBrushCollection..st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/menuGeometricCollection..st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/menuMiscCollection..st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/submenuDict..st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonetIcons.class/class/license.st Outdated Show resolved Hide resolved
ClFeSc
ClFeSc requested changes May 19, 2021
View reviewed changes
Copy link
Contributor

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

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

I didn't check that the icons are copied correctly. Everything I saw in Squeak looks fine so I assume the errors, if any, are not dramatic.

packages/SketchMorph2-Core.package/M2CircleBrush.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2ClearCanvas.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2Eraser.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2ExportMorph.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/M2ExportPNG.class/class/icon.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/menuGeometricCollection.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/menuMiscCollection..st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/menuMiscCollection.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/submenuDict..st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/properties.json Outdated Show resolved Hide resolved
ClFeSc
ClFeSc requested changes May 19, 2021
View reviewed changes
Copy link
Contributor

@ClFeSc ClFeSc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some newlines still missing. Newlines look good after commit ccc714d.

packages/SketchMorph2-Core.package/MorphicMonet.class/instance/chooseColor.st Outdated Show resolved Hide resolved
packages/SketchMorph2-Core.package/MorphicMonet.class/instance/defaultCanvasPanelBorder.st Outdated Show resolved Hide resolved
@maximilian-franz maximilian-franz requested a review from ClFeSc May 19, 2021 10:47
ClFeSc
ClFeSc reviewed May 19, 2021
View reviewed changes
Copy link
Contributor

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

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

Apart from the looping in the color selection I'm happy with the changes.

ClFeSc
ClFeSc approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor

@ClFeSc ClFeSc left a comment

Choose a reason for hiding this comment

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

I agree to leave the loop for now.

maximilian-franz and others added 14 commits May 20, 2021 11:17
@ClFeSc ClFeSc force-pushed the feature/34-pipette branch from 808f286 to 887c3dd Compare May 20, 2021 09:21
@ClFeSc ClFeSc merged commit 5c5abb3 into dev May 20, 2021
@ClFeSc ClFeSc deleted the feature/34-pipette branch May 20, 2021 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClFeSc ClFeSc ClFeSc approved these changes

@jannis-hpi jannis-hpi jannis-hpi approved these changes

@johannes-poetzsch johannes-poetzsch johannes-poetzsch approved these changes

@kpostnov kpostnov kpostnov approved these changes

@DevSchmidtchen DevSchmidtchen DevSchmidtchen approved these changes

Assignees

@DevSchmidtchen DevSchmidtchen

@kpostnov kpostnov

@johannes-poetzsch johannes-poetzsch

@jannis-hpi jannis-hpi

@maximilian-franz maximilian-franz

Labels
Sprint 2021.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pipetten-Tool (3)
6 participants
@maximilian-franz @kpostnov @DevSchmidtchen @johannes-poetzsch @jannis-hpi @ClFeSc