-
Notifications
You must be signed in to change notification settings - Fork 62
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
Added artist page, FIXES #361 #362
Conversation
β Deploy Preview for mogulchristmas ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are accessibility issues in these changes.
artists/index.html
Outdated
<!-- Toggler Outside Menu --> | ||
<div id="header"> | ||
<div id="switch"> | ||
<label role='switch' class="switch"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.
artists/index.html
Outdated
<div id="header"> | ||
<div id="switch"> | ||
<label role='switch' class="switch"> | ||
<input class="switch-input" type="checkbox" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.
artists/index.html
Outdated
<div id='grid-container' class="row"> | ||
<div class="flipper"> | ||
<div class="front"> | ||
<img src="../images/artists-imgs/1.jpg" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
artists/index.html
Outdated
|
||
<div class="flipper"> | ||
<div class="front"> | ||
<img src="../images/artists-imgs/2.webp" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
artists/index.html
Outdated
|
||
<div class="flipper"> | ||
<div class="front"> | ||
<img src="../images/artists-imgs/3.webp" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
artists/index.html
Outdated
|
||
<div class="flipper"> | ||
<div class="front"> | ||
<img src="../images/artists-imgs/8.jpg" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
artists/index.html
Outdated
|
||
<div class="flipper"> | ||
<div class="front"> | ||
<img src="../images/artists-imgs/9.webp" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
artists/index.html
Outdated
|
||
<div class="flipper"> | ||
<div class="front"> | ||
<img src="../images/artists-imgs/10.jpg" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
artists/index.html
Outdated
|
||
|
||
|
||
<img src="../images/artists-imgs/9.webp" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
artists/index.html
Outdated
|
||
|
||
<img src="../images/artists-imgs/9.webp" /> | ||
<img src="../images/artists-imgs/10.jpg" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing. Just need a few changes to get done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will mobile users find info on the artists (They can't hover over right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hover sometimes isn't smooth. Please have a look.
@@ -415,9 +415,13 @@ | |||
<a href="/you-may-like" | |||
>You May Also Like</a | |||
> | |||
<a href="/news-articles" target="_blank" | |||
<!-- <a href="/news-articles" target="_blank" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, don't remove this for now. As I said, I will consider removing this later on. But, this can go below on the nav-bar maybe after Rate Song?
images/artists-imgs/10.jpg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4th will do it, I actually was trying to find smaller pic hence was stuck with that..
@abhinav2712 any update? |
I may need some more time for this, just got into some other things, but will fix in coming time |
@abhinav2712 - any update? |
@abhinav2712 - any update or should I close the PR? |
@abhinav2712 why? |
@KendallDoesCoding Man, Have a lot of other commitments, didnt want to hold this PR, you can do one thing to add this in issue s FEAT REQUEST, or shall I do it? |
I'll reopen PR but I or someone else will work on it. I don't want to stress you out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π You fixed the issue(s)! Great work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM βοΈ.
This works for now, but we'll make it slightly more responsive in the future.
30a0aa0
into
KendallDoesCoding:main
π οΈ Fixes Issue
Fixes #361
Related Issue/Addition to code
π¨βπ» Changes proposed
Type of change
βοΈ Check List (Check all the applicable boxes)
π Note to reviewers
π· Screenshots
New Feature: