processing / processing-website

Repository for the processing.org website
https://processing.org
GNU General Public License v2.0
64 stars 95 forks source link

Update FilterBar.js #347

Closed mglst closed 1 year ago

mglst commented 1 year ago

This issue is about the Filter feature on the Documentation page. When I type, for example, "textal" to search for the page for textAlign, my computer (macOS 13.1, Safari 16.2) autocompletes to "tectal". I wish it didn't! Out of habit, I press the return key (by habit e.g. using Google) so my computer autocompletes, and I don't see the page I'm looking for. The modification I suggest fixes this. I think it makes sense that code should not be autocompleted, and most of what I type into the filter input is code / a processing function I know exists.

What I did was make a local change on my browser to verify the behaviour i.e. that adding spellcheck="false" actually disables my browser's / my computer's spellcheck. Then I found the FilterBar.js and modified it. I'm assuming this changes the generated html files. I have not tested this i.e. I have not run the site locally using npm.

SableRaf commented 1 year ago

Thanks @ThinkDumbIndustries for the PR ✨

Pinging @runemadsen: should the value of spellcheck be set in reference.js like the others?

runemadsen commented 1 year ago

Thanks for this! I think it's fine to change it in the FilterBar component, but the attribute will need to be spelled camel-case to work. @ThinkDumbIndustries Can you change the PR to add the attribute like this:

spellCheck={false}
mglst commented 1 year ago

@runemadsen I amended my PR as per your comment.

However I'm note sure about the camel-case spelling; all I see online and in the HTML web docs is lowercase spellcheck not spellCheck.

I'm also unsure about surrounding false with brackets instead of double quotation marks. From reading the handful of lines above the one I'm adding, it seems the values which depend on arguments passed to the FilterBar arrow function are surrounded by brackets - e.g. {placeholder} - and the values which are constants are surrounded by double quotation mars - e.g. name="search". In light of this, it seems to me as though spellcheck="false" would be correct.

Further, this has made me realise that FilterBar is a function and may be used in other places in the website. I did a quick search of the repo's code, and indeed it seems to be used in the SideBar component, on the Examples page, on the Reference page, and in the Reference page for the libraries. My suggested change has farther reaching consequences than I originally imagined! I'm now asking myself whether or not it is a good idea to disable spellcheck as a default behaviour for all of these pages. In my opinion, this would be appropriate for the two References pages, but maybe not for the examples page. Examples seem to be filtered by their titles, all of which seem rather human readable and worth of spellchecking. Finally, I'm unsure how SideBar makes use of FilterBar - maybe spellcheck makes sense there too. Maybe spellchecking should be an option passed as an argument to the FilterBar function, so as to be enable / disable on a case by case basis.

Thoughts?

runemadsen commented 1 year ago

Thanks @ThinkDumbIndustries. Please see answers below.

However I'm note sure about the camel-case spelling; all I see online and in the HTML web docs is lowercase spellcheck not spellCheck.

This is a React component and React components have camelcasing in their attributes. So what is classname and spellcheck in regular HTML is className and spellCheck in React.

I'm also unsure about surrounding false with brackets instead of double quotation marks. From reading the handful of lines above the one I'm adding, it seems the values which depend on arguments passed to the FilterBar arrow function are surrounded by brackets - e.g. {placeholder} - and the values which are constants are surrounded by double quotation mars - e.g. name="search". In light of this, it seems to me as though spellcheck="false" would be correct.

In React, booleans should be given as proper JavaScript values and therefore the curly brackets are warranted.

Further, this has made me realise that FilterBar is a function and may be used in other places in the website. I did a quick search of the repo's code, and indeed it seems to be used in the SideBar component, on the Examples page, on the Reference page, and in the Reference page for the libraries. My suggested change has farther reaching consequences than I originally imagined! I'm now asking myself whether or not it is a good idea to disable spellcheck as a default behaviour for all of these pages. In my opinion, this would be appropriate for the two References pages, but maybe not for the examples page. Examples seem to be filtered by their titles, all of which seem rather human readable and worth of spellchecking. Finally, I'm unsure how SideBar makes use of FilterBar - maybe spellcheck makes sense there too. Maybe spellchecking should be an option passed as an argument to the FilterBar function, so as to be enable / disable on a case by case basis.

Unless there are accessibility arguments for disabling the spell check, I would favor disabling it. @SableRaf Do you have thoughts here?

SableRaf commented 1 year ago

Unless there are accessibility arguments for disabling the spell check, I would favor disabling it. @SableRaf Do you have thoughts here?

I think disabling the spellcheck makes sense in that case. Once you validate the review, I'll merge the PR. Thanks for looking into it!