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

London|Phone Naing|Module-User-Focused-Data|Week1 #306

Closed
wants to merge 3 commits into from

Conversation

Mgphone
Copy link

@Mgphone Mgphone commented Nov 2, 2024

Learners, PR Template

Self checklist

  • [✅ ] I have committed my files one by one, on purpose, and for a reason
  • [ ✅] I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • [✅ ] I have tested my changes
  • [✅ ] My changes follow the style guide
  • [ ✅] My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

Copy link

netlify bot commented Nov 2, 2024

Deploy Preview for cyf-ufd ready!

Name Link
🔨 Latest commit 8eebd8f
🔍 Latest deploy log https://app.netlify.com/sites/cyf-ufd/deploys/672940b3679f6900078a218e
😎 Deploy Preview https://deploy-preview-306--cyf-ufd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Mgphone Mgphone changed the title Feature/wire frame London|Phone Naing|Module-User-Focused-Data|Week1 Nov 2, 2024
Copy link

@maesierra maesierra left a comment

Choose a reason for hiding this comment

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

Thank you very much. Good work. But I there a couple of small issues that I would like to point out.

  • If you look at the wireframe, the lower columns have the same height for the images so the text on both columns is aligned vertically. I can see how the second column have the title higher than the first one.
  • In the acceptance criteria, is mentioned that it should use first-child to style the first article. It's nothing wrong with your approach. It's perfectly valid to use different classes, but in this exercise we would like you to explore how you can achieve that using first-child
  • The footer is not fixed to the viewport. If you resize the window to something smaller you see how it's not showing at the bottom. Could you think of any changes to your css styling on how to achieve that?

index.html Outdated
</section>

<!-- Subsection 2 -->
<section class="subsection-second">

Choose a reason for hiding this comment

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

This section structure looks good, but it really helps a lot to read it if you increase the indentation for each section inside a section. It makes it much more readable if you format it this way:

    <section class="subsection-second">
            <section class="subsection">
                <img src=https://i.sstatic.net/e3dd7.jpg" alt="Why do developers need Git?">
                <h2>Why do developers need Git?</h2>
                <p>Git is essential for developers as it facilitates collaboration, allows tracking of code changes, and helps manage different versions of projects.</p>
                <button class="read-more-button" onclick="window.location.href='https://docs.github.com/en/get-started/start-your-journey/about-github-and-git'">Read More</button>
            </section>

            <!-- Subsection 3 -->
            <section class="subsection">
                <img src="https://media.geeksforgeeks.org/wp-content/uploads/20240226111013/image-258.webp" alt="What is a branch in Git?">
                <h2>What is a branch in Git?</h2>
                <p>A branch in Git allows developers to create separate lines of development, making it easier to work on features independently before merging them.</p>
                <button class="read-more-button" onclick="window.location.href='https://docs.github.com/en/get-started/start-your-journey/about-github-and-git'">Read More</button>
            </section>
     </section>

This way you have a clear way to know where a section starts and ends.
If you're using and IDE like VS Code, there is always the option to request the IDE to format it for you. In VS Code, the option is called Format Document

Copy link
Author

Choose a reason for hiding this comment

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

Thanks that idea is lovely :)

index.html Outdated Show resolved Hide resolved
@maesierra maesierra added the Reviewed Volunteer to add when completing a review label Nov 2, 2024
@Mgphone
Copy link
Author

Mgphone commented Nov 4, 2024

I did resolve all the issues

@Mgphone Mgphone added the Complete Participant to add when work is complete and review comments have been addressed label Nov 6, 2024
@maesierra
Copy link

Have you checked the results after your changes? I don't think they match the wireframe anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants