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

Fixes #350 Personalized homepage #891

Merged
merged 2 commits into from Dec 20, 2018
Merged

Fixes #350 Personalized homepage #891

merged 2 commits into from Dec 20, 2018

Conversation

keianhzo
Copy link
Collaborator

Fixes #350 Added a setting to set a homepage in the Developer options panel. Also we now provide an adb parameter to do it from the command line.

@keianhzo keianhzo requested a review from cvan December 14, 2018 09:48
Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

thank you very much for picking up where I left off 😄

testing more when I get a chance on Friday. here are a few nits in the meantime.

app/src/main/res/values/non_L10n.xml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

This is a great feature enhancement that's been asked for a few times by users. Glad to see this work - nice job! 👍

A few items to address:

  1. Text caret seems to be missing when I on press on a letter. (I think this is true for all editable text fields in the Settings panes.) It's not possible to alter characters without clearing the entire field and retyping a URL. (I was expecting similar text treatment as with the URL bar, <input> fields, etc.)

image

  1. Clearing the Homepage URL field does not work. Can we have the empty value have the placeholder text, Default Home, and when the user presses Save/Enter, we restore to https://webxr.today behind the scenes (but never render the https://webxr.today URL in the UI)?

image

  1. Pressing Reset Developer Options does not reset the homepage URL.

image

@keianhzo
Copy link
Collaborator Author

@cvan Thanks for the review! I have fixed the mentioned issues.

Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

Thanks. You addressed all the items nicely 👍

  1. Autofocus text field for Homepage field when Edit button is pressed.

Current

image
image

Expected

(Also, see item 4 below re: Firefox Home (Default))
image


  1. URL disappears when you double-press 2-3 times to the right of the text in the field. (The text and highlight colour appear both white.)

image


  1. Press+drag-and-holding on a segment of the text in the field should be white on navy instead of dark gray on blue.

image

Compare to double-pressing which shows white on dark gray:

image


  1. When Homepage is empty (hint) or the text value (default) is displayed, is it possible to print Firefox Home (Default) instead of https://webxr.today?

image

image

Context

I propose the simplest option today would be, instead of exposing https://webxr.today, we use the same string that is used on Firefox desktop: Firefox Home (Default)

When we have multiple tabs, windows, and other features for user customisation, we can revisit how to augment this.

Desktop

Firefox 64.0 macOS

image

Mobile

Firefox 14.0 (12646) for iOS


  1. The Homepage value gets properly reset upon pressing Reset Developer Options, and if the old Homepage is in the background, the page appears to reload, but it still shows the same Home. I'd expect to be taken to the Home page if I was on last page.

This might be the intentional behaviour, but it confused me a bit with the page appearing to reload (or perhaps it was a redraw; it was difficult to tell).

image

Similar thing happens when the Homepage URL gets originally modified to its non-default value:

image


Let me know if I should clarify anything.

Thank you! Excellent work on this. :+1

@keianhzo
Copy link
Collaborator Author

keianhzo commented Dec 18, 2018

@cvan Thanks one more time for the thorough review, it has helped to fix some other pre existing bugs. All 1-5 issues above should be fixed and working as proposed.

@keianhzo keianhzo force-pushed the custom-startpage branch 2 times, most recently from 177891b to 17f4596 Compare December 18, 2018 14:41
Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

Thanks for refining the details! This is shaping up quite nicely. 👍


image

  1. The hint text is correct, but could we have the same text (Firefox Home (Default)) in the non-edit mode?

image


  1. When I click the Homepage's Edit button, and I don't press Enter/Save, but then I press another field, can we close the Homepage editable mode and consider the action cancelled? (Let me know if that makes sense.)

@keianhzo
Copy link
Collaborator Author

@cvan

  1. I've set the hint for both the edit and non-edit modes, I think is more consistent that way.
  2. I've fixed all the focus related changes, now everything should get closed when focus changes.

Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

  1. I've set the hint for both the edit and non-edit modes, I think is more consistent that way.

nice, the hint and text are both correct.

one more nit: is it possible to disable editing of the text when the value is Firefox Home (Default)? specifically, is there a way to disable character selection and force only select-all / clear?

looking to avoid this situation:

image
image
image

  1. I've fixed all the focus related changes, now everything should get closed when focus changes.

this works perfectly! I love it.

Copy link
Contributor

@cvan cvan left a comment

Choose a reason for hiding this comment

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

👍well-tested and well-designed. rock solid work! I tested every case I could think of.

ship it! 🚢 😄

@keianhzo keianhzo merged commit 4b02aea into master Dec 20, 2018
@cvan cvan deleted the custom-startpage branch December 20, 2018 16:44
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.

None yet

2 participants