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.

Sign up for GitHub

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

Jump to bottom

feat: add MediaMetadata #2410

Merged
merged 4 commits into from
Feb 24, 2022
Merged

feat: add MediaMetadata #2410

merged 4 commits into from
Feb 24, 2022

Conversation

Hashen110
Copy link
Contributor

@Hashen110 Hashen110 commented Jan 27, 2022 •
edited
Loading

@Hashen110
Copy link
Contributor Author

Customize media notifications and playback controls outside the browser

@lofcz lofcz mentioned this pull request Feb 7, 2022
Closed
sampotts
sampotts reviewed Feb 12, 2022
View reviewed changes
Copy link
Owner

@sampotts sampotts left a comment

Choose a reason for hiding this comment

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

Thanks! I just had one question re: the browserslist change but that's it. The other comment is just a nitpick and can be ignored if you want.

package.json Outdated
Comment on lines 31 to 34
"browserslist": [
"cover 100%",
"not dead"
],
Copy link
Owner

Choose a reason for hiding this comment

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

What's the reason behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My IDE (webstorm) showed a warning that string is not a valid option for the browserslist.

Copy link
Contributor Author

@Hashen110 Hashen110 Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Before changing the browserslist

Before changing the browserslist

After changing the browserslist

After changing the browserslist

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, your IDE has a bug I guess. It's definitely valid and is widely used. The CSS property transition and align-items no longer need prefixing so your first screenshot (before the change) is correct.

src/js/controls.js
Comment on lines +1760 to +1761
// eslint-disable-next-line no-empty
} catch (e) {}
Copy link
Owner

@sampotts sampotts Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Usually, I just do this to avoid disabling the linting rule:

catch(e) {
  // Do nothing
}

@sampotts
Copy link
Owner

Also, it looks like there are merge conflicts since the other PR for markers was merged. If you could address those, that'd be awesome. Thanks again!

sampotts
sampotts reviewed Feb 24, 2022
View reviewed changes
Copy link
Owner

@sampotts sampotts left a comment

Choose a reason for hiding this comment

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

Thanks! I've reverted the browserslist change as discussed.

@sampotts sampotts merged commit 6bc447b into sampotts:master Feb 24, 2022
@sampotts
Copy link
Owner

Thanks for the work on this. It's included in v3.7.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sampotts sampotts sampotts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Hashen110 @sampotts