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

Exercise8 #12

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

Exercise8 #12

wants to merge 2 commits into from

Conversation

Herzogin
Copy link
Contributor

@Herzogin Herzogin commented Feb 2, 2021

Last one! :-)

Copy link
Contributor

@roschaefer roschaefer left a comment

Choose a reason for hiding this comment

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

party

Hey @Systems-Development-and-Frameworks/dirty_thirties you received 5/5 ⭐ and even one 🚀 in this submission. Congrats!

I saw two tailwind PRs so far (including yours) and I tend to really like that it does not bloat the markup and the DOM with unnecessary <div>s for layouting purposes. 👍

Here's what I have for you:

⭐ For a header and it's responsiveness (direct links vs. hamburger menu)

⭐ For a content section and it's content items

⭐ For a responsive content section (3-column vs 1-column layout)

⭐ For responsive content items (1-column vs. 2-column layout)

⭐ For a footer

🚀 For a sidebar containing all the navigation links. The sidebar is revealed when the hamburger menu button is clicked and can be closed.


The components directory contains your Vue.js Components.

_Nuxt.js doesn't supercharge these components._
Copy link
Contributor

Choose a reason for hiding this comment

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

You could have done us a favour and rebased this change out of the diff 😉

@@ -0,0 +1,33 @@
<template>
<div class="flex flex-row px-2 lg:flex-col">
Copy link
Contributor

Choose a reason for hiding this comment

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

You could have used semantic HTML here. E.g. <article>
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article

<nav
class="absolute inset-y-0 right-0 w-full max-w-md p-6 text-white bg-gray-800"
>
<a href="#" @click.prevent="close" class="inline-block mb-12">
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have better been a <button>. For accessibility, you could add aria-label: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-label_attribute

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.

None yet

3 participants