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

Mp messaging example #1479

Merged
merged 6 commits into from
Apr 16, 2020
Merged

Conversation

danielkec
Copy link
Contributor

No description provided.

@danielkec danielkec self-assigned this Mar 10, 2020
@danielkec danielkec added the reactive Reactive streams and related components label Mar 10, 2020
@danielkec danielkec added this to the 2.0.0 milestone Mar 10, 2020
@danielkec danielkec marked this pull request as ready for review March 11, 2020 12:21
@danielkec danielkec changed the title WIP: Mp messaging example Mp messaging example Mar 11, 2020
Copy link
Contributor

@romain-grecourt romain-grecourt left a comment

Choose a reason for hiding this comment

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

  • I'd like to avoid having these pictures in this repository.
  • It's meant to be fancy but it's actually not, it's probably a better idea to go for a simpler look
  • Check-in the javascript files in the example rather than referencing a gist

You could simplify the look by using just a blue background and hardcode the Helidon logo as SVG in the html directly.

@danielkec
Copy link
Contributor Author

@romain-grecourt Fancy is what I have been aiming for, is 140 kB really too much?

@romain-grecourt
Copy link
Contributor

It's trying to be fancy, but it's not. It's an example, keep it simple rather than lame, go to the point.
I really want to avoid duplicating assets from the website all over the place.

@m0mus
Copy link
Contributor

m0mus commented Mar 19, 2020

I personally like the design with all these animations. It makes it nice and dynamic. I don't see anything wrong with checking it in into repository. I agree it's better to put js files in the repository.

@romain-grecourt Are there any other concerns except that you don't like the design?

@romain-grecourt
Copy link
Contributor

It's hard enough to maintain a website and keep things consistent. Let's not copy stuff from the website all over the place.

I don't agree with examples being fancy, they should be simple and illustrate a point. Demos can be fancy, is this a demo ?

At the very least get rid of the website background, it's not meant to be reused.

@m0mus
Copy link
Contributor

m0mus commented Mar 19, 2020

I think that one of the purposes of having the graphics is that we can use it in Helidon related assets such as are presentations, demos and samples. I don't agree that it must be kept on the web site only. Why the website background cannot be reused?

@romain-grecourt
Copy link
Contributor

The re-usable assets are here: https://github.com/oracle/helidon-site/tree/master/graphics.

The website background was specifically designed for the parallax effect, it's not meant to be re-used ; I'm guessing that background.png was built using the individual layers of the parallax.

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
  * Remove background image
  * Code snippet generated locally

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
@danielkec
Copy link
Contributor Author

Sorry for rebase, background image is removed and snippet is generated locally
Peek 2020-03-25 10-44

Copy link
Contributor

@romain-grecourt romain-grecourt 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 making the changes, LGTM on the frontend part.

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
@danielkec danielkec added the messaging Reactive Messaging label Apr 14, 2020
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

LGTM

@danielkec danielkec merged commit 3268ab2 into helidon-io:master Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
messaging Reactive Messaging reactive Reactive streams and related components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants