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.

Sign up for GitHub

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

Jump to bottom

Replace Install/Download Engine Links #359

Merged
merged 5 commits into from
Feb 12, 2019
Merged

Replace Install/Download Engine Links #359

merged 5 commits into from
Feb 12, 2019

Conversation

ricardobaeta
Copy link
Contributor

Signed-off-by: Ricardo Baeta [email protected]

@ricardobaeta
Replace Download Engine Links
8c9de6f 
Signed-off-by: Ricardo Baeta <[email protected]>
@ricardobaeta ricardobaeta changed the title Replace Download Engine Links Replace Install/Download Engine Links Feb 11, 2019
@ricardobaeta ricardobaeta mentioned this pull request Feb 11, 2019
Closed
@ricardobaeta ricardobaeta requested a review from vcoisne February 11, 2019 09:52
@ricardobaeta ricardobaeta self-assigned this Feb 11, 2019
dpordomingo
dpordomingo reviewed Feb 11, 2019
View reviewed changes
Copy link
Contributor

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

LGTM but:

  • idk if I like the idea of not linking the docs from our landing... but technically ok.
  • idk if we should use the proper product name of source{d} Engine, or if we can keep using the short one, or if we should address it in a different PR.

hugo/layouts/partials/header.html Show resolved Hide resolved
hugo/layouts/partials/try-sourced.html Show resolved Hide resolved
dpordomingo
dpordomingo suggested changes Feb 11, 2019
View reviewed changes
hugo/layouts/engine/single.html Outdated Show resolved Hide resolved
@dpordomingo @ricardobaeta
Update hugo/layouts/engine/single.html
e2e97ac 
Signed-off-by: Ricardo Baeta <[email protected]>

Thanks David! Sorrry about the typo :)

Co-Authored-By: ricardobaeta <[email protected]>
dpordomingo
dpordomingo approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@dpordomingo dpordomingo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM from a technical pov.

deployed into staging, https://landing-staging.srcd.run/engine/

I only see a problem in desktop between 991px-1200px breakpoints, where the menu overlaps our logo:
image

I think we could address it in a new PR, but whatever you preffer is ok to me.

@ricardobaeta
Copy link
Contributor Author

You're correct @dpordomingo ! And on a separate PR, which is not as important as this one because:

image

marnovo
marnovo requested changes Feb 11, 2019
View reviewed changes
hugo/layouts/engine/single.html Outdated Show resolved Hide resolved
hugo/layouts/partials/header.html Outdated Show resolved Hide resolved
hugo/layouts/partials/try-sourced.html Show resolved Hide resolved
@marnovo
Copy link
Member

marnovo commented Feb 11, 2019

As for what @dpordomingo mentioned on documentation:

  • We have the link in Docs in the navbar
  • We have it on the open source page
  • We don't (but could) have it linked in the https://go.sourced.tech/engine-download page so that people who don't want to give their email still can find the documentation/repo during the user journey.

marnovo
marnovo approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@marnovo marnovo left a comment

Choose a reason for hiding this comment

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

LGTM but check w/ @dpordomingo why the CI build is failing before merging.

@marnovo
Copy link
Member

marnovo commented Feb 12, 2019 •
edited
Loading

Regarding the discussion on supported resolutions & browsers, please use this reference as there was a lot of thought and work put in it (we still need to PR it to the guide I guess).

vcoisne
vcoisne approved these changes Feb 12, 2019
View reviewed changes
Copy link
Contributor

@vcoisne vcoisne 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 :)

@dpordomingo
Copy link
Contributor

I restarted Drone.
If it pass, I'll merge and deploy as requested by @ricardobaeta

@dpordomingo dpordomingo merged commit 845f2f2 into master Feb 12, 2019
@dpordomingo dpordomingo deleted the replace-engine-download-links branch February 12, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marnovo marnovo marnovo approved these changes

@vcoisne vcoisne vcoisne approved these changes

@dpordomingo dpordomingo dpordomingo approved these changes

Assignees

@ricardobaeta ricardobaeta

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ricardobaeta @marnovo @dpordomingo @vcoisne