freeCodeCamp / CurriculumExpansion

Creative Commons Attribution Share Alike 4.0 International
313 stars 105 forks source link

learn-basic-string-and-array-methods-by-building-a-music-player-app #338

Closed RafaelDavisH closed 1 year ago

RafaelDavisH commented 1 year ago

Checklist:

closes #327 closes #325

This the first draft of the music player project. It needs more string and array methods.

DEMO HERE

String and Array methods implemented

other concepts

Issues addressed on this PR:

325

327

RafaelDavisH commented 1 year ago

Screenshot 2023-06-23 at 4 04 51 PM

Screenshot 2023-06-23 at 4 05 16 PM

bbsmooth commented 1 year ago

The playlist has several accessibility issues, the primary one being that you can't get to it with a keyboard. I don't think it would take much effort to increase the accessibility. I would be happy to help out with this.

Star rating systems on the other hand can definitely be made accessible but they take a lot more work. I'm wondering if it might be best to leave it out of this project as it might be too advanced and detract too much from what this course is trying to teach.

bbsmooth commented 1 year ago

Whoops, accidentally closed this. My bad.

Also, just want to add that I think this looks very cool.

RafaelDavisH commented 1 year ago

@bbsmooth, Thank you for taking the time and detailed the issues that need to be change. I'll start working on these issues, totally agree the suggestions, and I look forward for more input once I am done with these changes.

bbsmooth commented 1 year ago

I'm wondering about responsiveness. Technically, you should be able to narrow down to 320px and not have significant horizontal scrolling. There are some things we could do to make this more responsive at narrow widths, such as place the album cover above the controls and placing the artist name and play time below the song name. I know we can't cover everything here and perhaps we have to make some sacrifices. Is this something we want to consider?

P.S. Boy do I hate some of github's interfaces.

jdwilkin4 commented 1 year ago

Hi @RafaelDavisH !

I will have more time later on this weekend to dive into the project and leave feedback. But for the assets folder, all of the svgs seem to be a total of 3-4 lines. So maybe we could remove the assets folder altogether and just add the svgs right into the main html file.

Remember, by this point in the curriculum, the campers will not be adding any HTML or CSS. That will all be provided for them.

jdwilkin4 commented 1 year ago

@RafaelDavisH

I think for the songs arr, we can just have it in the main script.js file instead of creating a separate file for it.

We can have campers start to build out the array for a few steps and then provide the rest after that. We do something similar in the football team project.

RafaelDavisH commented 1 year ago

Hi @RafaelDavisH !

I will have more time later on this weekend to dive into the project and leave feedback. But for the assets folder, all of the svgs seem to be a total of 3-4 lines. So maybe we could remove the assets folder altogether and just add the svgs right into the main html file.

Remember, by this point in the curriculum, the campers will not be adding any HTML or CSS. That will all be provided for them.

Thank you @jdwilkin4 for your suggestions. I have moved the song array and deleted most of the svg images, except pause and play. These are swap when they are triggered.

bbsmooth commented 1 year ago

Those buttons that were converted from images to svgs don't have alt text any more so there is nothing for screen readers to announce. We have a few different options for fixing this.

  1. Add role="img" to the svg and then add a title element in the svg with the text we want announced for the button. For example, the Previous button would be:
    <button class="previous">
                <svg
                  width="24"
                  height="14"
                  viewBox="0 0 24 14"
                  fill="none"
                  xmlns="http://www.w3.org/2000/svg"
                  role="img"
                >
                  <title>Previous</title>
  2. Use sr-only text to label the button (and of course we would need to add the sr-only ruleset to the CSS). For example, the Previous button would be:
    <button class="previous">
                <span class="sr-only">Previous</span>
  3. Use aria-label to give the button an accessible name. For example, the Previous button would be:
    <button class="previous" aria-label="Previous">

Any of the above will work. I'll let others decide which would be best for the course. Personally, if I were coding this I would choose either 2 or 3. Option 1 is fine, but most screen readers will add the word "graphic" or "image" when announcing the button so you get some extra information you really don't need.

bbsmooth commented 1 year ago

I added the aria-current attribute to the button of the currently selected song. This will help screen reader users know which song is currently selected as they navigate the play list. I think it's a good accessibility enhancement to add since it is fairly simple.

Also, if we do use aria-current then we can get rid of the selected class completely and just use [aria-current="true"] to style the currently selected song in the playlist.

RafaelDavisH commented 1 year ago

@naomi-lgbt @jdwilkin4 something I forgot to address is what @bbsmooth said about if we should consider to make this project responsive. Its using CSS Grid and Flexbox but more for layout purposes WYAT?

https://github.com/freeCodeCamp/CurriculumExpansion/pull/338#issuecomment-1516826411

naomi-lgbt commented 1 year ago

Yes, we should aim for responsive - the project preview window is very small.