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

draw two house and a hill, fix styling #50

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amnindersingh12
Copy link

@amnindersingh12 amnindersingh12 commented Oct 19, 2022

Regarding #46

  • Add two houses
  • Add a hill
  • Attachments of the changes

Updated View

pic

Crash on drilling into hill (same is applicable for houses)

poc

@shiffman shiffman requested a review from alin256 October 19, 2022 12:33
@shiffman
Copy link
Member

Thank you for contributing @amnindersingh12!

@alin256
Copy link
Collaborator

alin256 commented Oct 19, 2022

@amnindersingh12 thank you for your contribution. Can you fix the roof of the right house? It looks off by some pixels.

@shiffman I will try to review and merge before the end of the week.

@amnindersingh12
Copy link
Author

Hi @alin256 , I have fixed that distorted pixel for the house. Thank you

@alin256
Copy link
Collaborator

alin256 commented Oct 21, 2022

@amnindersingh12, would your changes work with the 600x400 canvas that was there before?

Currently, the styling is slightly inconsistent between the desktop and mobile versions, and the instruction fonts have become too small on mobile.

@shiffman, maybe we should create a feature branch [CodingTrain:houses] to collaboratively refine it further before merging it into [main].

@alin256
Copy link
Collaborator

alin256 commented Oct 21, 2022

@amnindersingh12, do you want to create a pull request to CodingTrain:houses-feature

I can advertise it, e.g. in the Readme, if you want some help with the styling/scaling.

@amnindersingh12
Copy link
Author

@amnindersingh12, would your changes work with the 600x400 canvas that was there before?

Currently, the styling is slightly inconsistent between the desktop and mobile versions, and the instruction fonts have become too small on mobile.

@shiffman, maybe we should create a feature branch [CodingTrain:houses] to collaboratively refine it further before merging it into [main].

I have checked in mobile and desktop, the scaling is inconsistent. And my changes are not working with 600x400 canvas. Can you guide me through styling/scaling or provide any resources to learn more about scaling and mobile/desktop thing ?

@alin256
Copy link
Collaborator

alin256 commented Oct 21, 2022

@amnindersingh12 I commented regarding the scaling in CSS in the pull request to your branch that I made earlier today.

In the sketch itself, the scaling is adjusted down if needed during the setup:

  const allowedWidth = min(windowWidth, screen.width);
  if (allowedWidth > defaultWidth) {
    canvas = createCanvas(defaultWidth, defaultHeight);
  } else {
    ratio = allowedWidth / defaultWidth;
    canvas = createCanvas(defaultWidth * ratio, defaultHeight * ratio);
    // scale(ratio);
  }

@shiffman
Copy link
Member

@alin256 your plan sounds great to me!

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