Navigation Menu

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

fix: Outgoing add-on links (fix #1859) #1877

Merged
merged 2 commits into from Feb 25, 2017
Merged

Conversation

tofumatt
Copy link
Contributor

@tofumatt tofumatt commented Feb 23, 2017

fix #1859

Before

linkify

After

screen shot 2017-02-24 at 00 19 29
screen shot 2017-02-24 at 00 19 20

ref={(ref) => { this.homepageLink = ref; }}>
{homepage.replace(/^https?:\/\//, '')}
</a>
{addon.homepage ? <dt>{i18n.gettext('Add-on Links')}</dt> : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

The dt will only be shown when addon.homepage is available. Presumably it should now be on the basis of either a homepage link or support url?

@tofumatt tofumatt changed the title WIP: fix: Outgoing add-on links (fix #1859) fix: Outgoing add-on links (fix #1859) Feb 24, 2017
@tofumatt
Copy link
Contributor Author

Okay, updated with screenshots and better tests/code. All set for r?

@muffinresearch
Copy link
Contributor

I wonder - if a url was missing a sane protocol should we just bin it and not display it at all?

ref={(ref) => { this.homepageLink = ref; }}>
{homepage.replace(/^https?:\/\//, '')}
</a>
{homepage || supportUrl ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be cleaner to move these bits into separate snippets using if statements to avoid so much ternary in the JSX?

@muffinresearch
Copy link
Contributor

r+wc

@tofumatt
Copy link
Contributor Author

I wonder - if a url was missing a sane protocol should we just bin it and not display it at all?

There's no harm in doing it this way and we have to check either way... so might as well make the best of bad data. I suppose I could log the data, but realistically I think this is better UX than not showing it because our data went off somewhere.

@tofumatt tofumatt merged commit 89ff059 into master Feb 25, 2017
@tofumatt tofumatt deleted the improve-addon-links-1859 branch February 25, 2017 00:34
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.

URL should be truncated if too long and outgoing prefix removed from text
2 participants