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

Added Triangle Shape #4

Merged
merged 4 commits into from Aug 6, 2019
Merged

Conversation

makermelissa
Copy link

Enjoy! It doesn't have stroke at this time, but this is an initial working triangle.

@makermelissa makermelissa requested a review from a team August 1, 2019 23:29
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks for adding triangle! Just a suggestion to simplify the logic.

super().__init__(self._bitmap, pixel_shader=self._palette, x=smallest_x, y=y0)

# pylint: disable=invalid-name, too-many-locals, too-many-branches
def _helper(self, x0, y0, x1, y1, x2, y2):
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Author

Choose a reason for hiding this comment

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

It draws the filled triangle. I can rename.

adafruit_display_shapes/triangle.py Show resolved Hide resolved
@makermelissa
Copy link
Author

Changes made

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

I'm ok as is but have one other suggestion if you agree. I'd rename fill and outline to fill_color and outline_color because they read more as actions (aka functions) now. Furthermore, the names will make it tougher to add other properties like outline_width later. Food for thought. :-)

@makermelissa
Copy link
Author

makermelissa commented Aug 6, 2019

I used fill and outline to remain consistent with the other shapes. I don't want to rename all of the shapes because that would most likely break a bunch of code. Also, outline_width is called stroke on the other shapes, so that shouldn't be a problem. :)

@makermelissa makermelissa merged commit 2687554 into adafruit:master Aug 6, 2019
@makermelissa
Copy link
Author

The only other change I was considering making (which can totally be done in a future release without breaking anything) is to add a Line shape and subclass that for the triangle to reduce code.

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 7, 2019
Updating https://github.com/adafruit/Adafruit_CircuitPython_HX8357 to 1.1.0 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_HX8357#4 from makermelissa/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_TCA9548A to 0.2.1 from 0.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_TCA9548A#12 from caternuson/iss11
  > Merge pull request adafruit/Adafruit_CircuitPython_TCA9548A#10 from caternuson/iss9

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Shapes to 1.1.2 from 1.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#6 from makermelissa/master
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#5 from makermelissa/master
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#4 from makermelissa/master
  > fixed typo in rtd link
makermelissa pushed a commit that referenced this pull request Jul 22, 2020
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

2 participants