- Issue created by @the_g_bomb
- 🇬🇧United Kingdom the_g_bomb
I usually leave it as the default and change things if any issues can't be fixed.
- 🇬🇧United Kingdom the_g_bomb
I have fixed 2 typos: "buttom" and "continer" in the file js/skipto.min.js
I have added the other words that cspell reported to a new .cspell-project-words.txt file. - 🇩🇪Germany rkoller Nürnberg, Germany
Thanks a lot for all the work! I went through everything and tried to review as much as i was able to, a few comments and questions.
- most of the changes look like those to satisfy the linter by adjusting indentation and or adding commas at the end of arrays.
- explicitly setting array() isnt necessary? thought it would be, that was the reason i'Ve added thoses type. but is it only necessary for functions likepublic function buildForm(array $form, FormStateInterface $form_state): array {
but unncessary and or wrong within functions in the cases you've corrected?
'#options' => array(
- a good call renaming
generate_skipto_config_json
toskipto_generate_config_json
. looks more consistent
- and also good call to wrap the landmark labels intot()
functions. i am using my OS with english due to the fact that using voiceover in german is unbearable and i was under the impression that those landmark labels would use the english labels as well in consequence ^^. but i'Ve rechecked right now and they are localized. we have to check if there are official translations for those landmarks by the W3C cuz i wouldnt necessarily use landmarks used by a certain screen reader solution in a certain language. meaning, translations might differ inbetween screen readers in the worst case?
- the only thing i am uncertain and unable to check are the use statements you have removed in a view files. is there some automated test to check which are obsolete? or is that a manual task?so aside the use statements the MR would be RTBC from my end. the use statements i am unable to assess so it is up to you ^^ but guess it should be fine (manually tested aside running phpstan).
- 🇬🇧United Kingdom the_g_bomb
Thanks for the review. To answer your question, yes, I used phpcbf in places to fix anything that phpcs deemed as straightforward to fix. e.g. removing unused use statements, and it will usually pick up spacing and comment issues easily and to replace array() with [].
Others were manual but led by the error messages being generated by either phpcs or phpstan (e.g. naming convention for functions and making the select options translateable) to ensure best practices. For those, I had to manually implement a suitable solution.
All the changes implemented were to fix an error reported by the automated test tools.
- 🇬🇧United Kingdom the_g_bomb
You can see the previous jobs that were run here: https://git.drupalcode.org/project/skipto/-/jobs
Browsing back through the jobs, you will see which tools failed at which point, and you can also dig in if you wish to see the errors reported.