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 full width option for Adaptive card #6

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

msd117c
Copy link
Contributor

@msd117c msd117c commented Jan 2, 2024

No description provided.

@msd117c msd117c self-assigned this Jan 2, 2024

def initialize(body:, actions: nil, entities: [])
def initialize(body:, actions: nil, entities: [], full_width: false)
Copy link
Contributor

@huNt-FMJ huNt-FMJ Jan 2, 2024

Choose a reason for hiding this comment

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

I'm not a fan of the parameter name. I see that full_width is mapped/converted here, so the main value is a string called full or default.
What I would like to see is either differentiating this new API from the API/params that are used by the MSTeams endpoint or lean more into what the MSTeam JSON values ought to be.

The parameter changed just slightly by adding the prefix full.
However, the word full is used by Microsoft, here, so you combined the key + (potential) value, but converted it into a bool.

So either you use something like:
use_full_width, use_max_width, stretch_card_to_max_width
i.e. something that makes it more obvious that this parameter accepts a bool and is more explanatory about what it does
or
you let the user assign a constant, which you pre-define as a module.

module MSTAdaptiveCardWidth
  DEFAULT = 'default'
  FULL = 'full'

  def self.all
    [DEFAULT, FULL]
  end
end

where width can be (as of now), default and full.

Suggested change
def initialize(body:, actions: nil, entities: [], full_width: false)
def initialize(body:, actions: nil, entities: [], width: DEFAULT)

Then you can do MSTAdaptiveCardWidth::DEFAULT, while potentially even omit the MSTAdaptiveCardWidth prefix, if you define the module in this very same file.

The advantage is of course that you can basically delete the method set_width(full_width:).
On top of that extending this class to support a new size would be trivial.

@@ -23,6 +23,8 @@ def initialize(body:, actions: nil, entities: [])

@entities = entities
raise "AdaptiveCard `entities` must be an Array" unless @entities.nil? || @entities.is_a?(Array)

set_width(full_width: full_width)
Copy link
Contributor

@huNt-FMJ huNt-FMJ Jan 2, 2024

Choose a reason for hiding this comment

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

If you keep this method you should be able to omit the variable:

Edit:
I see we want to support Ruby 2.7.1, so my suggestion is invalid.

Copy link
Contributor

@huNt-FMJ huNt-FMJ Jan 2, 2024

Choose a reason for hiding this comment

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

The prefix set appears to be less fitting for me, because this getter/setter convention is not ruby-ish, rather Java-ish

{
type: "FactSet",
facts: [fact]
context "with full width" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
context "with full width" do
context "with full width set to `true`" do

end
end

context "with not full width" do
Copy link
Contributor

@huNt-FMJ huNt-FMJ Jan 2, 2024

Choose a reason for hiding this comment

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

Suggested change
context "with not full width" do
context "with full_width set to `false`" do

or

Suggested change
context "with not full width" do
context "with default width" do

MsTeamsHermes::Components::AdaptiveCard.new(body: [fact_set], actions: [action], full_width: false)
end

it "renders the hash object with default width" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it "renders the hash object with default width" do
it "renders the hash object including a width entry showing `default`" do

or

Suggested change
it "renders the hash object with default width" do
it "renders the hash object including a default width" do

MsTeamsHermes::Components::AdaptiveCard.new(body: [fact_set], actions: [action], full_width: true)
end

it "renders the hash object with full width" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it "renders the hash object with full width" do
it "renders the hash object including the width field set to `full`" do

Copy link
Contributor

@huNt-FMJ huNt-FMJ left a comment

Choose a reason for hiding this comment

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

Your call

@@ -14,7 +14,12 @@ module Components
class AdaptiveCard < Base
attr_reader :body, :actions, :entities, :width

def initialize(body:, actions: nil, entities: [], full_width: false)
WIDTH = {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work? To incorporate a "private" structure into this class such that any outside caller would still be able to
AdaptiveCard.new(body: xyz, with: default)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd encourage you to use a module to encapsulate said constants, instead of a hash.

https://stackoverflow.com/questions/75759/how-to-implement-enums-in-ruby

@@ -14,7 +14,12 @@ module Components
class AdaptiveCard < Base
attr_reader :body, :actions, :entities, :width

def initialize(body:, actions: nil, entities: [], full_width: false)
WIDTH = {
default: "default",
Copy link
Contributor

Choose a reason for hiding this comment

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

In ruby, when you want to create a constant, ruby encourages to capitalize them.

full: "full"
}.freeze

def initialize(body:, actions: nil, entities: [], width: WIDTH[:default])
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is to also use a guard to protect against someone using a not accepted constant/string.

raise "You can only use constants defined in  <MSTAdaptiveCardWidth>" unless MSTAdaptiveCardWidth.all.include? width

Copy link
Contributor

@huNt-FMJ huNt-FMJ left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@msd117c msd117c merged commit 8e63882 into main Jan 8, 2024
3 checks passed
@msd117c msd117c deleted the ms/full_width_adaptative_card branch January 8, 2024 08:29
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.

3 participants