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

Revert "Revert "Turn off input autocomplete"" #532

Closed
wants to merge 4 commits into from

Conversation

nomadturk
Copy link

index.html
Turned of autocomplete again. Removed spellcheck.
Added tabindex="1", before that when I clicked anywhere else on the page, tab button didn't go to text box at first try.
Replaced button with an input.

style.css
Changed show-more buttons position to relative to avoid it being displayed over the existing text.
Due to changing Send button to an input class, some adjustments were needed for it's positioning.

Turned of autocomplete again. Removed spellcheck.
Added tabindex="1", before that when I clicked anywhere else on the page, tab button didn't go to text box at first try.
@floogulinc floogulinc mentioned this pull request Nov 5, 2015
Changed show-more buttons position to relative to avoid it being displayed over the existing text.
Due to changing Send button to an input class, some adjustments were needed for it's positioning.
@nomadturk
Copy link
Author

Merged commits.

@JocelynDelalande
Copy link
Collaborator

@nomadturk Hi, and thanks for your contrib :)

So what's actualy the goal of the whole PR ?

hint: "Update index.html" is not a very helpfull commit message, cause meta-data already says it, it's better to adopt an intention/feature point of view while writing commits.

@nomadturk
Copy link
Author

Right, I didn't get into much detail since I was talked about it in irc but I forgot what happens when someone looks back to it a while later.

There are 3 main things changed:

1- This patch denies #518 and removes that. Because without autocomplete, writing suggestions for mobile keyboards was enabled but at the mean time also whenever you clicked on text area to write a new message, you were given a number of your last typed sentences. Especially on mobile, this was a huge buzzkill because after 3-5 sentences on any channel, these suggestions covered your whole screen.

So, in order to remove it I did some changes, ensuring writing suggestions still work.
I have added autocomplete=off and also added spellcheck=off, that might be removed if needed but the main reason I added that was barely anyone uses spellcheck on irc.

Along with these, I changed the "Send" button's type to input in the process. Due to these changes I needed to add/edit some css rules as mentioned above.

I also added tabindex=1 to make the text box the first tab in order to trap us more to the chat box.

Again, with this PR I tried fixing #366 by changing it's position to relative. It wasn't a pretty sight to see "Show more" bar on existing chat messages.

Lastly, when I last updated the files, I believe I didn't add anything so I believe it's only the meta data you see. Because what I did there was fixing my mistake of using spaces by replacing them again with tabs just like the rest of the file.

I think this was it.

@floogulinc
Copy link
Collaborator

Is it actually necessary to change the send button to an input element?

@JocelynDelalande
Copy link
Collaborator

Hi @nomadturk

Thanks for all that, there is a lot of good stuff, it's the PR hiding the bugfixes forest ;-)

1- This patch denies #518 and removes that. Because without autocomplete, writing suggestions for mobile keyboards was enabled but at the mean time also whenever you clicked on text area to write a new message, you were given a number of your last typed sentences. Especially on mobile, this was a huge buzzkill because after 3-5 sentences on any channel, these suggestions covered your whole screen.

So, in order to remove it I did some changes, ensuring writing suggestions still work.
I have added autocomplete=off

Ok, if I get it well, it will allow swiping/touch input to do suggestions, but disable suggestion of previous message that may cover the screen, right ?

and also added spellcheck=off, that might be removed if needed but the main reason
I added that was barely anyone uses spellcheck on irc.

I disagree, however it could be discussed, but in another PR, as it is another change.

Along with these, I changed the "Send" button's type to input in the process. Due to these changes I needed to add/edit some css rules as mentioned above.

Same question as @floogulinc , why is this needed ?

I also added tabindex=1 to make the text box the first tab in order to trap us more to the chat box.

Cool thing, but should go in another PR.

Again, with this PR I tried fixing #366 by changing it's position to relative. It wasn't a pretty sight to see "Show more" bar on existing chat messages.

Lastly, when I last updated the files, I believe I didn't add anything so I believe it's only the meta data you see. Because what I did there was fixing my mistake of using spaces by replacing them again with tabs just like the rest of the file.

That don't have to appear in git history, you have to merge the commit that added tabs with the one replacing them with spaces. From other contributors those as two separate commits are only noise.

I think this was it.

Ok, so to sum it up, I think it's valuable contribution, but that needs to be reorganized, please

git rebase -i and git add -p are your friends :-)

Once that is done, it could get merged/discussed feature by feature… and I will be all in favor :) (except spellchecking disabling).

@astorije
Copy link
Collaborator

astorije commented Nov 8, 2015

@JocelynDelalande seems to have taken first review for this PR, I'll take second review when he is OK with it. Thanks!

@astorije
Copy link
Collaborator

@JocelynDelalande, any news regarding review for this PR?

@nomadturk
Copy link
Author

Hey there...

Sorry, I'm out of the city and away from internet since a while and I'm busy till the end of this month.

I am, however, planning to comply with the recommended changes above and submitting several PR's instead of one big chunky PR when I have time.

@JocelynDelalande
Copy link
Collaborator

@nomadturk cool :), waiting for your edits then, don't hesitate to mention my name on newly created PR, so that it triggers some notification to my inbox, for review.

@astorije
Copy link
Collaborator

astorije commented Jan 8, 2016

Hey @nomadturk, I see that you haven't got the time to get back to this PR :-(
Do you think you'll still be able to wrap it up? That fix would mean a lot for Shout!
And no worries for the delay/silence, we are all busy so we know how it goes ;-)

nomadturk added a commit to nomadturk/shout that referenced this pull request Jan 8, 2016
Updated the form to not auto complete what we wrote before by removing action="" and adding autocomplete="off"
Making so now, especially when I'm on mobile I don't get my screen filled with previously typed sentences.

This commit is related to erming#532
@nomadturk
Copy link
Author

OK,

I have created only two PR's for now.
Sorry for taking too long.

I really don't recall why I changed the Send button to an input as well as input type of it. Maybe it was to make them look better. I'll do further tests and make a PR if need be.

Spellcheck, well, I personally chose not to share words all the time. Sometimes it's really disturbing to see your messages being corrected. But well, since it was a personal call, I didn't think people will want it. So, I won't be adding it.

As for tabindex, I tried it on IE, Chrome, Firefox and it was locked into the text box. So, cancel that as well.

That's all. We shall continue at the new PR #'s.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants