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

workshop page improved #820

Merged
merged 7 commits into from Sep 28, 2020
Merged

workshop page improved #820

merged 7 commits into from Sep 28, 2020

Conversation

s-pratyush
Copy link
Contributor

@s-pratyush s-pratyush commented Sep 25, 2020

Description

This PR fixes #783

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

@welcome
Copy link

welcome bot commented Sep 25, 2020

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Contributors' Welcome Guide and sure to join the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while peforming a commit.

@netlify
Copy link

netlify bot commented Sep 25, 2020

Deploy preview for layer5io ready!

Built with commit 99513ec

https://deploy-preview-820--layer5io.netlify.app

Pratyush-Saxena added 5 commits September 25, 2020 13:59
Signed-off-by: Pratyush-Saxena <pratyush.19b131054@abes.ac.in>
Signed-off-by: Pratyush-Saxena <52444607+Pratyush-Saxena@users.noreply.github.com>
Signed-off-by: Pratyush-Saxena <saxena18prats@gmail.com>
Signed-off-by: Pratyush-Saxena <pratyush.19b131054@abes.ac.in>
Signed-off-by: Pratyush-Saxena <52444607+Pratyush-Saxena@users.noreply.github.com>
Signed-off-by: Pratyush-Saxena <saxena18prats@gmail.com>
Signed-off-by: Pratyush-Saxena <pratyush.19b131054@abes.ac.in>
Signed-off-by: Pratyush-Saxena <52444607+Pratyush-Saxena@users.noreply.github.com>
Signed-off-by: Pratyush-Saxena <saxena18prats@gmail.com>
Signed-off-by: Pratyush-Saxena <pratyush.19b131054@abes.ac.in>
Signed-off-by: Pratyush-Saxena <52444607+Pratyush-Saxena@users.noreply.github.com>
Signed-off-by: Pratyush-Saxena <saxena18prats@gmail.com>
Signed-off-by: Pratyush-Saxena <pratyush.19b131054@abes.ac.in>
Signed-off-by: Pratyush-Saxena <52444607+Pratyush-Saxena@users.noreply.github.com>
Signed-off-by: Pratyush-Saxena <saxena18prats@gmail.com>
@Neilblaze
Copy link
Contributor

Neilblaze commented Sep 25, 2020

@Pratyush-Saxena nice work!

Before I approve the PR, I am suggesting some changes on the above.

  • In mobile view, the workshops.svg image is at second position & it's dimension is also minimized. Sharing a screenshot for reference. This needs to be fixed.

mob_workshop

Shift the workshops.svg image at the first position & increase it's size.
It'll be great if you add media queries to neglect the workshops.svg image for mobile view! [Optional]

  • In your deploy preview of the workshop page, add a padding of 50px between the starting row & the workshop text.

  • Fix the position of the forklift in the newsletter section.

Rest LGTM 👍

Copy link
Contributor

@Neilblaze Neilblaze left a comment

Choose a reason for hiding this comment

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

Remove this two auto-generated files! [.gitpod.Dockerfile & .gitpod.yml]

Copy link
Contributor

@Neilblaze Neilblaze left a comment

Choose a reason for hiding this comment

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

Reset the Ruby version to 2.7.0

<img
class="activator"
src="{{workshop.img}}"
style="
Copy link
Contributor

Choose a reason for hiding this comment

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

Shift the inline css to a fixed .css file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Neilblaze so in which file should i shift this styling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shift the inline css to a fixed .css file.

or i think it should be just removed because ...i tried so and found nothing being affected..

Copy link
Contributor

Choose a reason for hiding this comment

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

if the above css does nothing then feel free to remove it, if not then try shifting this to assets/css/<filename.css>

@Neilblaze
Copy link
Contributor

Also, adding to the above,

#783 is already resolved!
#773 looks perfect 👌

@s-pratyush
Copy link
Contributor Author

s-pratyush commented Sep 26, 2020

@Pratyush-Saxena nice work!

Before I approve the PR, I am suggesting some changes on the above.

  • In mobile view, the workshops.svg image is at second position & it's dimension is also minimized. Sharing a screenshot for reference. This needs to be fixed.

mob_workshop

Shift the workshops.svg image at the first position & increase it's size.
It'll be great if you add media queries to neglect the workshops.svg image for mobile view! [Optional]

  • In your deploy preview of the workshop page, add a padding of 50px between the starting row & the workshop text.
  • Fix the position of the forklift in the newsletter section.

Rest LGTM

@Neilblaze i can make all the changes you requested except one thing that making workshop.svg display at the top in mobile view which is not possible coz these cards are displayed using a sequestial condition

@Neilblaze
Copy link
Contributor

Neilblaze commented Sep 26, 2020

@Pratyush-Saxena do one thing. Break the sequence. Keep the workshop.svg image just below the "Workshop" text. Then start the sequence from the row present just below the workshop.svg . Make sure to keep a distinct gap / padding between them.

@s-pratyush
Copy link
Contributor Author

@Pratyush-Saxena do one thing. Break the sequence. Keep the workshop.svg image just below the "Workshop" text. Then start the sequence from the row present just below the workshop.svg . Make sure to keep a distinct gap / padding between them.

That's what I have already done.....but the point is that how could i do that for mobile view and tab view separately

@Neilblaze
Copy link
Contributor

See if you do what I said, then the sequence will be like this -

suggestion_workshop

Now I think it should be clear!

@s-pratyush
Copy link
Contributor Author

Yeah I guess now it's pretty much clear to me .

Signed-off-by: Pratyush-Saxena <52444607+Pratyush-Saxena@users.noreply.github.com>
<img
src="/assets/images/workshops/workshops.svg"
style="overflow: hidden;"
style="overflow: hidden;max-height:80vh"
Copy link
Contributor

Choose a reason for hiding this comment

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

add margin-top: 25px; to this line

@Neilblaze
Copy link
Contributor

Umm maybe it'll look better if you remove the top workshop illustration & keep the bottom workshop illustration as it is, since keeping same illustration twice in a single page might not look that good.

Amazing work by the way @Pratyush-Saxena 👌
Make the quick change & I'll approve the PR!

Signed-off-by: Pratyush-Saxena <pratyush.19b131054@abes.ac.in>
Copy link
Contributor

@Neilblaze Neilblaze left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Nice work 🎉

Make sure to remove the gitpod files, since they are unnecessary auto-generated files.

@s-pratyush
Copy link
Contributor Author

s-pratyush commented Sep 27, 2020

LGTM
Nice work

Make sure to remove the gitpod files, since they are unnecessary auto-generated files.

where do you find these files..pls let me know

@Neilblaze
Copy link
Contributor

Here

@s-pratyush
Copy link
Contributor Author

s-pratyush commented Sep 27, 2020

Here

@Neilblaze they were deleted in commit 371a02f alredy and are now not a part of my base repository

@Neilblaze
Copy link
Contributor

Oh I see, I forgot to upstream. Ok then it's good to go!

@s-pratyush
Copy link
Contributor Author

Oh I see, I forgot to upstream. Ok then it's good to go!

when will it get merge?

@Nikhil-Ladha
Copy link
Contributor

Which issues are being fixed here? I see the link in the description points to a merged PR, please correct that and also don't mention issue names in the commit subject. Also, is the issue #773 fixed?

Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Looking good, @Pratyush-Saxena! 💥

@leecalcote leecalcote merged commit a7b0a77 into layer5io:master Sep 28, 2020
@welcome
Copy link

welcome bot commented Sep 28, 2020

Thanks for your contribution to the Layer5 community! 🎉

Congrats!
        ⭐ Please star the project if you have yet to do so.

@leecalcote
Copy link
Member

Whoo-hoo, @Pratyush-Saxena! 🎈

Thank you, @Neilblaze 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants