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

Prevent prependHttp from failing if url is not defined #17928

Merged
merged 2 commits into from
Oct 16, 2019
Merged

Prevent prependHttp from failing if url is not defined #17928

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17d260f
Check that url is defined before passing into prependHttp
Oct 14, 2019
c82630b
Shift check from component to url lib
Oct 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/format-library/src/link/inline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ class InlineLinkUI extends Component {
onKeyPress={ stopKeyPropagation }
url={ url }
onEditLinkClick={ this.editLink }
linkClassName={ isValidHref( prependHTTP( url ) ) ? undefined : 'has-invalid-link' }
Copy link
Member

Choose a reason for hiding this comment

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

I think we should fix prependHTTP instead.

* @param {string} url The URL to test.

In its definition, it's expected to pass string so it should return the original value otherwise. It'd be similar to what addQueryArgs does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer the fix to prependHTTP as a separate PR to the UI enhancements that @senadir has suggested?

Copy link
Member

Choose a reason for hiding this comment

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

UI enhancement can be filed as a separate issue. We should fix the bug in the first place to make it usable :)

Copy link
Member

@gziolo gziolo Oct 15, 2019 •
edited

Choose a reason for hiding this comment

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

I tested it with the Class block and it looks like the way it's handled there is, it gets never converted into a link if you leave the input field empty. In the case where you remove the URL and save it, it works as if you would remove the link. We should replicate the same behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the PR to just return the original value in prependHTTP instead. This fixes the bug - not sure if we want to do a proper check for a string value rather than a simple truthy check.

With removing the link if url is not set, this is slightly complicated by the fact that we need to account for existing named anchor tags that will have no url attribute. The Classic block does account for these, and it has a different UI treatment for anchors. Do we want to combine looking at this in a separate PR along with the UI enhancements mentioned above?

Copy link
Contributor

Choose a reason for hiding this comment

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

My feeling is that it's probably best to get this fix in as is, and then look to do other improvements separately. The current fix improves things quite a bit, and leaves only a couple of rough edges (which is way better than a crashing block).

Bearing in mind there's some work (#17846) to revamp the link interface going on separately, it might be best to create a separate issue and seek some design feedback on how to handle this use case. It can then be factored in to the new design.

What do think @gziolo?

Copy link
Member

Choose a reason for hiding this comment

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

UI enhancement can be filed as a separate issue. We should fix the bug in the first place to make it usable :)

Nothing has changed. Let's get it in 👍

Copy link
Member

Choose a reason for hiding this comment

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

Filed a follow-up: #17972

linkClassName={ url && isValidHref( prependHTTP( url ) ) ? undefined : 'has-invalid-link' }
/>
) }
</URLPopoverAtLink>
Expand Down