Skip to content

Shaun Harris #162

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Shaun Harris #162

wants to merge 1 commit into from

Conversation

Shaun-Harris
Copy link

Not having the responsiveness I would like.

Comment on lines +19 to +26
<div class="logo"><svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="x-svg"><g><path d="M18.244 2.25h3.308l-7.227 8.26 8.502 11.24H16.17l-5.214-6.817L4.99 21.75H1.68l7.73-8.835L1.254 2.25H8.08l4.713 6.231zm-1.161 17.52h1.833L7.084 4.126H5.117z"></path></g></svg></div>
<div class="functions">
<div class="home"><svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="logo-svg"><g><path d="M21.591 7.146L12.52 1.157c-.316-.21-.724-.21-1.04 0l-9.071 5.99c-.26.173-.409.456-.409.757v13.183c0 .502.418.913.929.913H9.14c.51 0 .929-.41.929-.913v-7.075h3.909v7.075c0 .502.417.913.928.913h6.165c.511 0 .929-.41.929-.913V7.904c0-.301-.158-.584-.408-.758z"></path></g></svg><p><strong>Home</strong></p></div>
<div class="explore"><svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="logo-svg"><g><path d="M10.25 3.75c-3.59 0-6.5 2.91-6.5 6.5s2.91 6.5 6.5 6.5c1.795 0 3.419-.726 4.596-1.904 1.178-1.177 1.904-2.801 1.904-4.596 0-3.59-2.91-6.5-6.5-6.5zm-8.5 6.5c0-4.694 3.806-8.5 8.5-8.5s8.5 3.806 8.5 8.5c0 1.986-.682 3.815-1.824 5.262l4.781 4.781-1.414 1.414-4.781-4.781c-1.447 1.142-3.276 1.824-5.262 1.824-4.694 0-8.5-3.806-8.5-8.5z"></path></g></svg><p>Explore</p></div>
<div class="notifications"><svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="logo-svg"><g><path d="M19.993 9.042C19.48 5.017 16.054 2 11.996 2s-7.49 3.021-7.999 7.051L2.866 18H7.1c.463 2.282 2.481 4 4.9 4s4.437-1.718 4.9-4h4.236l-1.143-8.958zM12 20c-1.306 0-2.417-.835-2.829-2h5.658c-.412 1.165-1.523 2-2.829 2zm-6.866-4l.847-6.698C6.364 6.272 8.941 4 11.996 4s5.627 2.268 6.013 5.295L18.864 16H5.134z"></path></g></svg><p>Notifications</p></div>
<div class="messages"><svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="logo-svg"><g><path d="M1.998 5.5c0-1.381 1.119-2.5 2.5-2.5h15c1.381 0 2.5 1.119 2.5 2.5v13c0 1.381-1.119 2.5-2.5 2.5h-15c-1.381 0-2.5-1.119-2.5-2.5v-13zm2.5-.5c-.276 0-.5.224-.5.5v2.764l8 3.638 8-3.636V5.5c0-.276-.224-.5-.5-.5h-15zm15.5 5.463l-8 3.636-8-3.638V18.5c0 .276.224.5.5.5h15c.276 0 .5-.224.5-.5v-8.037z"></path></g></svg><p>Messages</p></div>
<div class="lists"><svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="logo-svg"><g><path d="M3 4.5C3 3.12 4.12 2 5.5 2h13C19.88 2 21 3.12 21 4.5v15c0 1.38-1.12 2.5-2.5 2.5h-13C4.12 22 3 20.88 3 19.5v-15zM5.5 4c-.28 0-.5.22-.5.5v15c0 .28.22.5.5.5h13c.28 0 .5-.22.5-.5v-15c0-.28-.22-.5-.5-.5h-13zM16 10H8V8h8v2zm-8 2h8v2H8v-2z"></path></g></svg><p>Lists</p></div>
<div class="bookmarks"><svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="logo-svg"><g><path d="M4 4.5C4 3.12 5.119 2 6.5 2h11C18.881 2 20 3.12 20 4.5v18.44l-8-5.71-8 5.71V4.5zM6.5 4c-.276 0-.5.22-.5.5v14.56l6-4.29 6 4.29V4.5c0-.28-.224-.5-.5-.5h-11z"></path></g></svg><p>Bookmarks</p></div>
Copy link

Choose a reason for hiding this comment

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

This is incredibly difficult to read and maintain, I can't really tell where one SVG begins and another ends. You should try to keep things formatted nicely so it's easy to read:

<div class="logo">
    <svg fill="#E7E9EA" viewBox="0 0 24 24" aria-hidden="true" class="x-svg">
        <g>
            <path d="M18.244 2.25h3.308l-7.227 8.26 8.502 11.24H16.17l-5.214-6.817L4.99 21.75H1.68l7.73-8.835L1.254 2.25H8.08l4.713 6.231zm-1.161 17.52h1.833L7.084 4.126H5.117z"></path>
        </g>
    </svg>
</div>

Copy link
Author

Choose a reason for hiding this comment

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

Hi Nathan, Will take this on board. I think in this case all the

's were confusing me in in terms of alignment (which I found easier if on one line). However, I think today's lesson should eliminate that by using different semantics. So I know and can clearly see that there is an opening and closing to the semantics by having varied names rather than consistent
s which make it harder to read.

Do you want me to alter this file and resubmit or just apply this feedback to work going forward?

Copy link

Choose a reason for hiding this comment

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

No need to update this, just feedback to keep in mind going forward :)

Comment on lines +75 to +77



Copy link

Choose a reason for hiding this comment

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

I don't think we need this much white space

Copy link
Author

Choose a reason for hiding this comment

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

Again I think this is a tool for me as I have dyslexia and sometimes require the space. If you have a better solution that could be helpful such as text size etc. that would be great.

Copy link

Choose a reason for hiding this comment

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

Ah, you could try going to File -> Preferences -> Settings in VS Code and finding the Editor: Font Size setting. Bump that up to a value you feel comfortable with, hopefully that may help things.

You could also continue to use this white space approach while developing the solution, and then just remove it all when you're done so you still get the helpfulness of it and it doesn't make its way into the final product

Copy link

@vherus vherus left a comment

Choose a reason for hiding this comment

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

Your work is good but I think you could tidy it up a bit, this would be a little difficult to maintain in a real project

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.

2 participants