Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Set bookmark url if no title present fixes #3442 #3445

Closed
wants to merge 2 commits into from
Closed

Set bookmark url if no title present fixes #3442 #3445

wants to merge 2 commits into from

Conversation

Sh1d0w
Copy link

@Sh1d0w Sh1d0w commented Aug 26, 2016

cc @bridiver

@luixxiul luixxiul changed the title Set bookamrk url if no title present fixes #3442 Set bookmark url if no title present fixes #3442 Aug 26, 2016
@bsclifton
Copy link
Member

bsclifton commented Aug 26, 2016

I think it's important to keep the title as empty. Other parts of the code reference that (about:history, long pressing on the back button to show history).

Our bookmarks code is smart enough to try to render using (in this order):

  1. customTitle
  2. title
  3. location (URL)

Would we be able to tweak this instead to set the URL as a placeholder on the the input box (and then make OK button enabled?) I think this would be the best fix

screen shot 2016-08-26 at 10 51 04 am

@bridiver
Copy link
Collaborator

chrome uses the url as the title, but I definitely see the problem now with my change. If you try to create an empty title it will revert to the url

@bridiver bridiver added the design A design change, especially one which needs input from the design team. label Aug 26, 2016
@bridiver
Copy link
Collaborator

cc @bradleyrichter

@Sh1d0w
Copy link
Author

Sh1d0w commented Aug 27, 2016

@bridiver @bsclifton I have changed the logic there. Now the button will be disabled only if you try to create folder with an empty name.

For bookmarks, it will default to the website title or url as a placeholder text just like @bsclifton suggested.

There is one small problem tho and it is in this file:

https://github.com/brave/browser-laptop/blob/master/js/state/siteUtil.js#L112

As you can see, once bookmarked forever bookmarked, so if you bookmarked one website with customTitle then delete it and try to bookmark it again, the placeholder text may be different from what will be saved as default. How do you prefer to handle this? Do you want me to check for old custom title to display it in the placeholder text?

@bbondy
Copy link
Member

bbondy commented Aug 27, 2016

When I add a new bookmark now it shows this:
screenshot 2016-08-27 00 10 32

That's not ideal because it shows the title as placeholder text and doesn't allow you to edit it.

I think we should just leave it blank and close this. Having the url as placeholder text kind of makes sense because that's the value you'll see when it's created, but it is kind of confusing too because it's like you are suggesting a url as the title.

@bbondy bbondy closed this Aug 27, 2016
@bbondy
Copy link
Member

bbondy commented Aug 27, 2016

yep sorry I was DM'ing with @bridiver on Slack about that, he's reverting that too but allowing save with a blank title.

@bsclifton
Copy link
Member

Being able to save with a blank title is ideal, since the renderer can choose what to show

Thanks for working through this one with us, @Sh1d0w 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design A design change, especially one which needs input from the design team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants