italia / designers.italia.it

Designers Italia mette a disposizione la conoscenza e gli strumenti per progettare e realizzare servizi pubblici centrati sulle necessità delle persone
https://designers.italia.it
BSD 3-Clause "New" or "Revised" License
53 stars 36 forks source link

Remove all the linting warnings #1108

Open bfabio opened 7 months ago

bfabio commented 7 months ago

There are a few linting warning remaining that pollute our npm run lint output and are most likely symptoms of actual bugs , we should fix those - preferably not by disabling them:

❯ npm run lint

> designers.italia.it@2.0.0a lint
> eslint --ignore-path .gitignore .

/home/fabio/dev/designers.italia.it/src/components/checkbox/checkbox.js
  18:5  warning  Visible, non-interactive elements with click handlers must have at least one keyboard listener                                                                                                     jsx-a11y/click-events-have-key-events
  18:5  warning  Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs to an interactive content element  jsx-a11y/no-static-element-interactions

/home/fabio/dev/designers.italia.it/src/components/content-collapse/contentCollapse.js
  49:7  warning  Anchor used as a button. Anchors are primarily expected to navigate. Use the button element instead. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid

/home/fabio/dev/designers.italia.it/src/components/feedback/components/form-no/FormNo.js
   85:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
   94:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  103:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  112:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  121:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  130:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  139:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  148:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  157:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  166:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  175:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  197:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  206:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  215:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  224:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  233:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  242:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  251:15  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control

/home/fabio/dev/designers.italia.it/src/components/feedback/feedback.js
   63:9   warning  Unexpected console statement                    no-console
  132:19  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control
  144:19  warning  A form label must be associated with a control  jsx-a11y/label-has-associated-control

/home/fabio/dev/designers.italia.it/src/components/footer-main/footer-main.js
  72:45  warning  'index' is already declared in the upper scope on line 66 column 33  no-shadow

/home/fabio/dev/designers.italia.it/src/components/list-item/list-item.js
   95:26  warning  'icons' is already declared in the upper scope on line 92 column 7                                                                                                                                                                                                                                                                                                 no-shadow
  122:26  warning  The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. If you cannot provide a valid href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid

/home/fabio/dev/designers.italia.it/src/components/media-player/media-player.js
  106:11  warning  Media elements such as <audio> and <video> must have a <track> for captions  jsx-a11y/media-has-caption

/home/fabio/dev/designers.italia.it/src/components/nav-sidebar/nav-sidebar.js
  200:15  warning  The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. If you cannot provide a valid href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid

/home/fabio/dev/designers.italia.it/src/components/search-main/search-main.js
  113:51  warning  'index' is already declared in the upper scope on line 31 column 25  no-shadow
  137:23  warning  A form label must be associated with a control                       jsx-a11y/label-has-associated-control

/home/fabio/dev/designers.italia.it/src/components/section-editorial/section-editorial.js
  107:25  warning  The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. If you cannot provide a valid href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid
  112:25  warning  The href attribute requires a valid value to be accessible. Provide a valid, navigable address as the href value. If you cannot provide a valid href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md  jsx-a11y/anchor-is-valid

/home/fabio/dev/designers.italia.it/src/components/table/table.js
  29:36  warning  'index' is already declared in the upper scope on line 27 column 35  no-shadow
  37:53  warning  'index' is already declared in the upper scope on line 29 column 36  no-shadow

✖ 35 problems (0 errors, 35 warnings)
Fupete commented 7 months ago

Update: this PR https://github.com/italia/designers.italia.it/pull/1112 fixed half of the warnings as you can see:

updat

@bfabio I leave the correction of :

Finally I have a couple of doubts about media-player.js, perhaps we should consider disabling this lint:

if (subtitles)
      video.player.addRemoteTextTrack({
        kind: "subtitles",
        label: "Italiano",
        srclang: "it",
        default: true,
        src: subtitles,
      });

Update: we need to investigate further the warning A form label must be associated with a control jsx-a11y/label-has-associated-control beause each labels seems to have its correct htmlFor property...

Fupete commented 7 months ago

⚠️ I have also found a potential a11y bug of Bootstrap Italia @astagi here: https://github.com/italia/designers.italia.it/commit/44db4306bc4dd66e2c93fad5271a028587334c7b . There was an <a> instead of the correct <button> semantic for the close action of the mobile version of the nav-sidebar...

Update: just open an issue on Bootstrap italia: https://github.com/italia/bootstrap-italia/issues/1029

Fupete commented 7 months ago

@bfabio I just updated the comment above after merging the PR that fix half of the warnings.

bfabio commented 1 month ago

Finally I have a couple of doubts about media-player.js, perhaps we should consider disabling this lint:

  • some of the old archive media we retrieved are without subtitles;

  • however, for the new ones, we load subtitles via the dedicated API method of video.js (line 53) within useEffect as you can see here:

if (subtitles)
      video.player.addRemoteTextTrack({
        kind: "subtitles",
        label: "Italiano",
        srclang: "it",
        default: true,
        src: subtitles,
      });

Thinking about it, videos not having the subtitles should be handled as an error IMO. So, the possible steps:

  1. Create an issue to subtitle all the old videos
  2. Keep the non blocking lint as a compromise, which can be use in the transition period
  3. Once all the videos are subtitled, switch that lint to an hard error