Skip to content

SoundCloud Widget: avoid undefined when Visual option is not set#2926

Closed
jeherve wants to merge 1 commit into
masterfrom
fix/undefined-soundcloud-visual
Closed

SoundCloud Widget: avoid undefined when Visual option is not set#2926
jeherve wants to merge 1 commit into
masterfrom
fix/undefined-soundcloud-visual

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Oct 29, 2015

@jeherve jeherve added Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Oct 29, 2015
@jeherve jeherve added this to the 3.8.1 milestone Oct 29, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you be more verbose about why you changed the logic here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The previous logic didn't appear to work at all in my tests. The SoundCloud widget always used the Visual appearance, regardless of your settings.

I'd be happy to rework on that logic completely though, to make it more readable maybe?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, I was just curious to know the reason. Now that I have tested the visual/previous player, I realize there was a problem here, thanks for fixing it!

@zinigor zinigor added [Status] Tested on WP.com [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 18, 2015
@dereksmart
Copy link
Copy Markdown
Contributor

I can't reproduce this issue on master using the OP's shortcode or various versions of it. No notices in error lor or on page. Am I missing something?

@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Nov 19, 2015

@dereksmart It's been a while since I tested this, but I seem to recall that I could trigger the notice by playing with the iFrame parameter in the shortcode.

@dereksmart
Copy link
Copy Markdown
Contributor

/shrug I can't break it. If you have a broken instance, can you paste the exact shortcode you're using?

@dereksmart dereksmart added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Nov 19, 2015
@dereksmart dereksmart modified the milestones: 3.8.2, 3.8.1 Nov 19, 2015
@jeherve
Copy link
Copy Markdown
Member Author

jeherve commented Nov 24, 2015

@dereksmart I still haven't been able to reproduce on any of my test sites. Here is the shortcode used by the site owner who reported the issue:

[soundcloud url="https://api.soundcloud.com/tracks/122514535" params="color=ff6600&auto_play=false&show_artwork=true" width="100%" height="166" iframe="true" /]

@dereksmart
Copy link
Copy Markdown
Contributor

Closing as no one can reproduce anymore. Please reopen if that changes or we get more reports.

@dereksmart dereksmart closed this Dec 3, 2015
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Dec 3, 2015
@samhotchkiss samhotchkiss deleted the fix/undefined-soundcloud-visual branch February 3, 2017 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Extra Sidebar Widgets [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] Tested on WP.com Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants