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

Made the navbar responsive down to 350px #35

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

Conversation

JoseBambora
Copy link
Contributor

Made the navbar mobile friendly, fixed a typo error and changed the name "performancelive" to "karaoke_performance_id" in order to be more intuitive of what it represent.

Copy link
Member

@ruioliveira02 ruioliveira02 left a comment

Choose a reason for hiding this comment

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

If I click on the go to button on the home page nothing happens and the button disappears. This happens only in the unauthenticated pages.

image
I think the dropdown should start right after the button ends vertically, as to not have any overlap which makes it a bit awkward to use

Copy link
Member

@ruioliveira02 ruioliveira02 left a comment

Choose a reason for hiding this comment

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

image
On mobile it is off center (there is some extra padding coming from the <ul> tags.

Comment on lines 92 to 94
<ul>
<a href={subentry.url}><%= subentry.title %></a>
</ul>
Copy link
Member

Choose a reason for hiding this comment

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

You should not be using <ul> here. If anything, it should be a <li> tag and you put the <ul> on top of the for loop

<ul>
  <%= for .... do %>
    <li>
      ...
    </li>
  <% end %>
</ul>

Copy link
Member

@ruioliveira02 ruioliveira02 left a comment

Choose a reason for hiding this comment

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

Make sure to format / lint your code so the checks pass

Comment on lines 17 to 60
<script>
var isClicked = false
function pixeis(){
var result = false
if(window.innerWidth < 700)
result = true
return result
}
function scene1()
{
document.getElementById("menu2").className = "text-center"
document.getElementById("content").className = "hidden"
document.getElementById("menu").className = "hidden"
}
function scene2()
{
document.getElementById("menu").className = "text-center"
document.getElementById("menu2").className = "hidden"
document.getElementById("content").className = "container"
}
function clickedbutton() {
isClicked = !isClicked
if(isClicked){
if(pixeis())
scene1();
else
scene2();
}
else{
document.getElementById("menu").className = "hidden"
document.getElementById("menu2").className = "hidden"
document.getElementById("content").className = "container"
}
}
function resized()
{
if(isClicked)
if(pixeis())
scene1()
else
scene2()
}
window.onresize = resized;
</script>
Copy link
Member

Choose a reason for hiding this comment

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

I am not a fan of this. Maybe we can use Alpine.js and Tailwind's breakpoints to clean this up (@nelsonmestevao)

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be really cool to use Alpine and tailwind breakpoints. Should I add alpine to this project?

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think it would be a good addition

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