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

Support 2017/11/30 "New options for template message images" #90

Merged
merged 3 commits into from
Jan 10, 2018

Conversation

clsung
Copy link
Contributor

@clsung clsung commented Dec 31, 2017

Support the following image options for buttons/carousel template
message:

  • imageAspectRatio
  • imageSize
  • imageBackgroundColor

CL Sung added 2 commits December 31, 2017 21:06
Support the following image options for buttons/carousel template
message:
- imageAspectRatio
- imageSize
- imageBackgroundColor
Copy link
Member

@be-hase be-hase 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 your PR and sorry for late response...
I wrote comment. Please check 🙏

@@ -100,7 +100,10 @@ class ButtonsTemplate(Template):
Template message with an image, title, text, and multiple action buttons.
"""

def __init__(self, text=None, title=None, thumbnail_image_url=None, actions=None, **kwargs):
def __init__(self, text=None, title=None, thumbnail_image_url=None,
Copy link
Member

Choose a reason for hiding this comment

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

Would you like to set the default value to None?

image_aspect_ratio=None, image_size=None, image_background_color=None

(I do not think that, ) if the default value of API changes, it will be necessary to modify the SDK as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.

However, currently in model/base.py, function as_json_dict. For those keys with value None will produce "" value when encoded as json.
I'm not sure if sending empty value with keys like imageSize="", imageAspectRatio="" will cause any problem (such as overwrite server-side default value.)

Anyway I'll update the pull request. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for agreeing. :D

Humm...
Currently I think None value is serialized as null.
ref:
https://github.com/line/line-bot-sdk-python/blob/master/tests/models/test_base.py#L32-L68

( And server-side uses default when value is null. )

@@ -161,12 +180,24 @@ class CarouselTemplate(Template):
Template message with multiple columns which can be cycled like a carousel.
"""

def __init__(self, columns=None, **kwargs):
def __init__(self, columns=None, image_aspect_ratio="rectangle",
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -217,7 +250,8 @@ class CarouselColumn(Base):
https://devdocs.line.me/en/#column-object
"""

def __init__(self, text=None, title=None, thumbnail_image_url=None, actions=None, **kwargs):
def __init__(self, text=None, title=None, thumbnail_image_url=None,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

@clsung clsung left a comment

Choose a reason for hiding this comment

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

address commnets

Copy link
Member

@be-hase be-hase left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks :D

@be-hase be-hase merged commit 4ad5e82 into line:master Jan 10, 2018
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.

2 participants