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

Updated sound library and readjusted sound library dialog to fit all categories #27348

Merged
merged 24 commits into from Mar 12, 2019

Conversation

kimj42
Copy link
Contributor

@kimj42 kimj42 commented Mar 4, 2019

  • What changed:

    • What's new: total of 1328 files along with their metadata JSON files were added to the soundLibrary.json after running code-dot-org/tools/scripts/rebuildSoundLibraryManifest.rb.
    • Source: Those files were retried from the dropbox folder that Jael has created and categorized, https://www.dropbox.com/sh/xwwh3p2lgmg9mwr/AADft87sqhvWkduV_qsBxbFoa?dl=0. 2 folders from the dropbox had the same folder name, Transition, so those files were combined to one folder in the s3 cdo-sound-library folder. Total of 28 new categories were created and are visible to the user if this change gets merged to staging. The Animals category already existed so I did not have to add that to the SoundLibrary.jsx but the new sounds files for that category have been added with this PR.
    • New categories names (28): 'Accent', 'Achievements', 'Alerts', 'App', 'Bell', 'Collect', 'Digital', 'Explosion', 'Hits', 'Human', 'Jump', 'Loops', 'Movement', 'Music', 'Nature', 'Notifications', 'Points',
      'Poof', Pop', 'Projectile', 'Puzzle', 'Retro', 'Slide', 'Swing', 'Swish', 'Tap', 'Transition', 'Whoosh'.
    • Only added new sounds and did not modify or remove any existing sounds.
  • Where it changed:

    • This PR affects Game Lab and App Lab. It's because those apps use a sound library dialog.
    • Before PR:

image

  • After merging PR, looks the same for Game Lab and App Lab:

screen shot 2019-03-04 at 12 19 05 pm

screen shot 2019-03-04 at 12 19 11 pm

screen shot 2019-03-04 at 12 30 21 pm

screen shot 2019-03-04 at 12 35 47 pm

  • Update, Sprite Lab also uses sound library dialog and dialog is showing up as expected:

screen shot 2019-03-04 at 1 31 18 pm

  • Follow-up:
    • Possibly working on the category names as Brad mentioned to localize the strings.
    • Need to wait for approval to merge this PR due to some license issues for the new sounds.
    • Once Ryan approves, he will write a comment to this PR then I will merge.

…ransition will be uploaded in a diff commit because of how the original folders with .wav files were separated
… doesn't have scroll feature available and it needs to be 3 column wide according to Ryan so will now work on that
…common.scss so that it applies to both gamelab and applab
@islemaster
Copy link
Contributor

Hi Karis! Thank you for adjusting your approach and moving the updates into one pull request (PR)! Since we're consolidating here, let's take the time to make this PR as great as possible.

When writing a pull request description, it's helpful to reviewers (and to our future selves!) to include some more details. In a lot of ways, our PRs are the best documentation we have. In this case, I'd love to see the following:

  • A detailed answer to what changed:
    "Updated sound library" could mean a lot of things. Will you add some details that help capture the scope of this change? For example:
    • How many new sounds?
    • How many new categories?
    • Where did the sounds come from?
    • Did we change or remove any sounds, or only add new ones?
  • A complete look at where it changed:
    Which parts of the site does this change affect? Is the sound library currently used only in App Lab, or does it affect Game Lab, Sprite Lab, and other apps as well? It's often a good idea to include a screenshot for context as well, especially when something visual changed (like your CSS changes to the sound library dialog).
  • Why are we making this change?
    The "why" is often the hardest thing to reconstruct when reading through changes years from now, so it's good practice to capture the original intent in pull request descriptions (or code comments!). This might be a simple one-sentence description, or it might be a detailed look at the metrics driving us toward making a particular change. It usually also involves links to relevant resources: To a spec, if you've got one; a Jira item; related pull requests (you've already done this, yay! 🎉); if it's a bug, maybe a link to a Zendesk ticket or Honeybadger error report.
  • A description of how you tested the change:
    If you've added automated tests to verify your change, at least mentioning that there's present is a good idea. If you haven't (and in this case I think that's fine), please give a detailed description of the manual testing you've done and your level of confidence that everything is working as expected. That's especially valuable with content changes like this, where I don't trust my ability to accurately review your 40,000 lines of code, and I'd like to know what mental model you had for checking your work. It's also useful documentation for the future - if there's ever an issue with the sound library, we might be able to use your testing steps to narrow it down!
  • What's next?
    If there's any follow-up work for this feature, it's helpful to list next steps, call out problems you're intentionally not addressing yet, that sort of thing. Or if this is really the last bit of work for the sound library (for now), call that out too! 🎉 Making it clear that a thing is done is helpful when reviewing past changes.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

soundLibrary.json jumped from 130KB to 2.1MB in this change. I'm a little worried about that. Interested in @joshlory's thoughts on this too - maybe time to fundamentally change our approach to this library? If so, are we okay with this as an intermediate state?

apps/style/common.scss Show resolved Hide resolved
@epeach
Copy link

epeach commented Mar 4, 2019

Not ship-blocking, but I get the grid illusion when looking at the expanded categories. If it's not just me, I wonder if there's a way to arrange the category boxes to remove the illusion.

@islemaster
Copy link
Contributor

Fantastic description, thank you for taking the time to write up all those details! This will be sooo helpful next time we have a sound library update.

My remaining concern is the size of soundLibrary.json, let's have a discussion about that before moving forward (maybe while Ryan is still working out licensing issues).

+1 to @epeach's grid illusion comment, I also get that. Let's track that as follow-up work as well instead of addressing it here; that seems to be a problem that existed before your changes.

@epeach
Copy link

epeach commented Mar 4, 2019

^Followed up with Mark on Slack re: grid illusion. He's doing some mock-ups and I'll add as bug to *Labs backlog.

@kimj42
Copy link
Contributor Author

kimj42 commented Mar 4, 2019

No, thank YOU, Brad, for providing such valuable feedback!!! I really appreciate it! I'll use your comment as a checklist for myself whenever I write up PRs to make sure it's a descriptive PR.

@kimj42
Copy link
Contributor Author

kimj42 commented Mar 5, 2019

Follow-up after discussion with Erin, Josh, and Brad:

  • Concluded with few options:
    • Gzipping before shipping
    • Change filenames to all lowercase and remove unnecessary words from aliases to reduce file size
    • Try to use references instead
    • Use IconLibrary
    • A test failed so I will work on passing that test
    • Work on localizing strings for category names

Josh will work on gzipping and I will check in with Ryan about lowercasing the filenames.

  • UPDATE: Josh noted this is a known issue which infrastructure is working on. Sound library is good to go without fixing this issue right now.

@kimj42
Copy link
Contributor Author

kimj42 commented Mar 6, 2019

Update on discussion with Ryan regarding lowercasing and cleaning up aliases:

  • There are about a total of 39? unique "string" aliases for these 1300 or so new sound files. Each aliases vary from having around 5/7 words to like more than 20 words.
  • Unnecessary words exist in those aliases so Ryan will manually take out those words then resend me the document with the cleaned up aliases so that I can recreate those sound metadata/JSON for each of those files.
  • New sounds will include "noResale" in their aliases to indicate they are new sound files.
  • Yes, sound names should all be lowercased.

…e the or epic media. sound files are all lowercase now to match existing style
…filename styles. category_app was not updated to have lowercase names so updated that as well.
…ecause it wasn't considering the new sound files that were added.
…had multiple underscores instead of one so changed that to one underscore.
@kimj42 kimj42 merged commit 32bd3b5 into staging Mar 12, 2019
@fisher-alice fisher-alice deleted the updating-soundLibrary branch July 13, 2022 22:21
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