Skip to content

Initial implementation#1

Merged
javier-godoy merged 11 commits into
masterfrom
initial-implementation
Aug 28, 2023
Merged

Initial implementation#1
javier-godoy merged 11 commits into
masterfrom
initial-implementation

Conversation

@paodb

@paodb paodb commented Aug 1, 2023

Copy link
Copy Markdown
Member

No description provided.

@paodb paodb requested a review from javier-godoy August 1, 2023 19:41
Comment thread src/main/java/com/flowingcode/vaadin/addons/shareeasy/BaseShareEasy.java Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread src/main/resources/META-INF/resources/frontend/styles/static_addon_styles Outdated
Comment thread src/main/resources/META-INF/resources/frontend/styles/style.css
Comment thread src/test/resources/META-INF/frontend/styles/share-easy-demo-styles.css Outdated
Comment thread src/test/java/com/flowingcode/vaadin/addons/shareeasy/it/BaseShareEasyIT.java Outdated
@paodb paodb force-pushed the initial-implementation branch 2 times, most recently from f7530fa to 2c9d302 Compare August 3, 2023 17:56
@paodb paodb requested a review from javier-godoy August 3, 2023 19:10

@javier-godoy javier-godoy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are several non-atomical commits. Instead of feat: implement A followed by fix: replace A with B just squash it and do feat: implement B, as if you had initially implemented B.

For instance:
refactor: move style file to an add-on specific location
squash with the commit where you commit the file to the other location

refactor(demo): update classnames to be more specific
squash with the commit where you implement the classnames

fix: make a defensive copy of customdrivers map
squash with the commit where you didn't make the defensive copy

fix: update customDrivers iteration on createSharee method
squash with the commit where you implement the iteration

style: remove blank lines
squash with the commit where those blank lines were added

fix: update parameters on executeJs calls
squash with the commit where the executeJs call is added

(and maybe others)

Comment thread src/test/java/com/flowingcode/vaadin/addons/shareeasy/it/BaseShareEasyIT.java Outdated
@paodb paodb force-pushed the initial-implementation branch 2 times, most recently from d3f03dc to ddd0a2a Compare August 14, 2023 17:00
@paodb paodb requested a review from javier-godoy August 14, 2023 17:03
@paodb paodb force-pushed the initial-implementation branch 2 times, most recently from 2f64cf5 to 41fa28b Compare August 16, 2023 14:34
@mlopezFC

Copy link
Copy Markdown
Member

I reviewed the addon and everything LGTM

@paodb paodb force-pushed the initial-implementation branch from 41fa28b to f9087d1 Compare August 23, 2023 12:26
@paodb paodb requested review from flang and mlopezFC August 23, 2023 12:27
@flang

flang commented Aug 23, 2023

Copy link
Copy Markdown

LGTM

LTR, RTL;
}

public String copiedSuccessfully = "Copied successfully";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The same observation applies #1 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

public String shareText;

/* By default current page's url */
public String shareLink;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The same observation applies #1 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

@paodb paodb requested a review from javier-godoy August 23, 2023 16:13

@mlopezFC mlopezFC left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@flang flang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@javier-godoy javier-godoy merged commit f872fe7 into master Aug 28, 2023
@javier-godoy javier-godoy deleted the initial-implementation branch August 28, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

4 participants