Add basic lints to the HTML macro. #1748

teymour-aldridge · 2021-02-17T14:26:12Z

This adds some basic accessibility-related lints to the HTML macro.

(Part of) #1334

I have run cargo make pr-flow
I have reviewed my own code
I have added tests

teymour-aldridge · 2021-02-17T14:38:50Z

I thought I'd broken something, but this seems to be failing in other pull requests as well :D

github-actions · 2021-02-17T15:14:44Z

Visit the preview URL for this PR (updated for commit bfcb04c):

https://yew-rs--pr1748-alt-text-lint-efwbd9d4.web.app

_{(expires Sun, 28 Nov 2021 13:04:21 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

teymour-aldridge · 2021-02-17T16:37:13Z

I'm also confused about the unit tests because they work on my local machine (with the identical toolchain).

siku2

This is amazing!

packages/yew-macro/src/lib.rs

packages/yew-macro/src/html_tree/html_block.rs

packages/yew-macro/src/html_tree/lint/mod.rs

siku2 · 2021-02-18T16:00:33Z

Did you manage to gather some feedback yet?

teymour-aldridge · 2021-02-18T17:22:38Z

Working on it :D

teymour-aldridge · 2021-02-18T17:22:56Z

This is amazing!

Thanks 😊

siku2

Apart from these relatively small issues I think this initial version is ready to be merged

packages/yew-macro/src/html_tree/html_dashed_name.rs

.github/workflows/pull-request.yml

packages/yew-macro/tests/html_macro_test.rs

packages/yew-macro/src/html_tree/lint/mod.rs

ranile

Lints look great

packages/yew-macro/src/html_tree/lint/mod.rs

lukechu10

Really excited for those!

ci/dictionary.txt

lukechu10 · 2021-03-03T05:14:58Z

packages/yew-macro/tests/html_lints/fail.rs

+
+fn main() {
+    let bad_a = html! {
+        <a>{"I don't have a href attribute"}</a>


Nit: I think the official style used by Yew has spaces between the { and the quotes (e.g. <a>{ "I don't have a href attribute" }</a>. Although #1774 would make this redundant.

teymour-aldridge · 2021-06-25T20:05:24Z

This should be ready for another review :D

mc1098

This is great, can't wait to see what other lints could spring from these!
Hope the review helps :)

packages/yew-macro/src/html_tree/lint/mod.rs

mc1098 · 2021-09-25T12:18:34Z

As mentioned in #1334 this is not considered blocked - @teymour-aldridge I know you mentioned about lack of reviewing power but I'm ready and able :)
Resolve the conflicts and ping me and it will be on the top of my review list ❤️

voidpumpkin · 2021-11-20T21:03:05Z

I had to make yew_router::Link accept queries with this update to the PR but all works now

mc1098

Small nit-picks from me 🙃

I generally would prefer seeing the router changes in a different PR - seems odd to stick them in a PR adding lints.

examples/router/src/main.rs

packages/yew-router/src/components/link.rs

ranile

Can we also console.warn these lints at runtime when compiled in debug mode? That way, people who don't use nightly will also be able to see the warnings

.github/workflows/pull-request.yml

packages/yew-macro/Makefile.toml

packages/yew-macro/src/html_tree/lint/mod.rs

ranile · 2021-11-21T08:23:36Z

packages/yew-macro/src/html_tree/lint/mod.rs

+                                "'#' is not a suitable value for the `href` attribute. \
+                                        Without a meaningful attribute assistive technologies \
+                                        will struggle to understand your webpage."


Can we link to documentation or add help information like clippy or cargo check do? A link to docs about ARIA roles would be nice to have

I added simple links to https://developer.mozilla.org/en-US/docs/Learn/Accessibility/HTML#onclick_events but i think in the future this can be improved upon

packages/yew-macro/tests/html_lints/fail.rs

voidpumpkin · 2021-11-21T13:00:38Z

Can we also console.warn these lints at runtime when compiled in debug mode? That way, people who don't use nightly will also be able to see the warnings

@hamza1311 Since this PR was not my initial idea, i would prefer to merge what we have now, and if you want you can improve it later by making it log to console.warn

NickHu · 2021-12-22T18:24:48Z

Is there any way to opt out of certain lints, like for clippy we have #[allow(clippy::...)]?

ranile · 2021-12-22T18:28:17Z

Is there any way to opt out of certain lints, like for clippy we have #[allow(clippy::...)]?

No, not right now.

Also, it would be better if you comment on the tracking issue instead of this already merged PR.

siku2 reviewed Feb 18, 2021

View reviewed changes

packages/yew-macro/src/lib.rs Outdated Show resolved Hide resolved

packages/yew-macro/src/html_tree/html_block.rs Outdated Show resolved Hide resolved

packages/yew-macro/src/html_tree/lint/mod.rs Outdated Show resolved Hide resolved

siku2 added the macro Issues relating to our procedural or declarative macros label Feb 18, 2021

teymour-aldridge force-pushed the alt-text-lint branch from 3647c54 to cd69bd5 Compare February 18, 2021 18:17

Xavientois mentioned this pull request Feb 20, 2021

Add support for the unit struct in Properties derive #1752

Merged

3 tasks

siku2 reviewed Feb 24, 2021

View reviewed changes

teymour-aldridge force-pushed the alt-text-lint branch from 24d0664 to 2f9c6a7 Compare February 26, 2021 14:20

ranile reviewed Feb 27, 2021

View reviewed changes

packages/yew-macro/src/html_tree/lint/mod.rs Outdated Show resolved Hide resolved

lukechu10 reviewed Mar 3, 2021

View reviewed changes

teymour-aldridge force-pushed the alt-text-lint branch from 3ce5917 to 9e5e620 Compare April 20, 2021 19:26

teymour-aldridge force-pushed the alt-text-lint branch from 9e5e620 to cd0edda Compare May 31, 2021 10:44

teymour-aldridge force-pushed the alt-text-lint branch 2 times, most recently from 2d1d751 to 3d5d2a7 Compare June 21, 2021 11:13

mc1098 reviewed Jul 16, 2021

View reviewed changes

packages/yew-macro/src/html_tree/lint/mod.rs Outdated Show resolved Hide resolved

packages/yew-macro/src/html_tree/lint/mod.rs Outdated Show resolved Hide resolved

teymour-aldridge force-pushed the alt-text-lint branch from d46598e to 2d1fbc1 Compare July 16, 2021 17:18

mc1098 mentioned this pull request Jul 31, 2021

Emit Warning when not using shorthand syntax #1979

Open

mc1098 added the A-yew-macro Area: The yew-macro crate label Sep 20, 2021

mc1098 linked an issue Sep 25, 2021 that may be closed by this pull request

Tracking issue for lints for the html! macro. #1334

Open

3 tasks

mc1098 removed a link to an issue Sep 25, 2021

Tracking issue for lints for the html! macro. #1334

Open

3 tasks

mc1098 added the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Sep 25, 2021

teymour-aldridge added 3 commits November 20, 2021 20:08

Add basic lints to the HTML macro.

2f3f1b6

f

e9fbec4

Fix the examples.

3128552

teymour-aldridge and others added 3 commits November 20, 2021 20:13

apply code review suggestions

8320915

update error message

7907772

rebase onto master

8c41db3

voidpumpkin force-pushed the alt-text-lint branch from bc69f95 to 8c41db3 Compare November 20, 2021 21:02

voidpumpkin removed the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Nov 20, 2021

remove unused props

ea7c49d

voidpumpkin previously approved these changes Nov 20, 2021

View reviewed changes

voidpumpkin enabled auto-merge (squash) November 20, 2021 21:11

voidpumpkin requested review from siku2, mc1098 and ranile November 20, 2021 21:11

mc1098 reviewed Nov 20, 2021

View reviewed changes

examples/router/src/main.rs Outdated Show resolved Hide resolved

examples/router/src/main.rs Outdated Show resolved Hide resolved

packages/yew-router/src/components/link.rs Outdated Show resolved Hide resolved

remove console log

2a37cf8

voidpumpkin dismissed their stale review via 2a37cf8 November 20, 2021 23:06

voidpumpkin disabled auto-merge November 20, 2021 23:11

ranile reviewed Nov 21, 2021

View reviewed changes

voidpumpkin added 2 commits November 21, 2021 14:22

remove js void

796bd01

fix according to PR comments

bfcb04c

voidpumpkin requested review from ranile and mc1098 November 21, 2021 14:22

voidpumpkin approved these changes Nov 21, 2021

View reviewed changes

voidpumpkin enabled auto-merge (squash) November 21, 2021 14:23

ranile approved these changes Nov 21, 2021

View reviewed changes

voidpumpkin merged commit 7e2542c into yewstack:master Nov 21, 2021

ranile mentioned this pull request Nov 21, 2021

Tracking issue for lints for the html! macro. #1334

Open

3 tasks

voidpumpkin mentioned this pull request Jan 11, 2022

Implement a way to opt-out from yew lints #2360

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic lints to the HTML macro. #1748

Add basic lints to the HTML macro. #1748

teymour-aldridge commented Feb 17, 2021 •

edited

Loading

teymour-aldridge commented Feb 17, 2021

github-actions bot commented Feb 17, 2021 •

edited

Loading

teymour-aldridge commented Feb 17, 2021

siku2 left a comment

siku2 commented Feb 18, 2021

teymour-aldridge commented Feb 18, 2021

teymour-aldridge commented Feb 18, 2021

siku2 left a comment

ranile left a comment

lukechu10 left a comment

lukechu10 Mar 3, 2021

teymour-aldridge commented Jun 25, 2021

mc1098 left a comment

mc1098 commented Sep 25, 2021

voidpumpkin commented Nov 20, 2021

mc1098 left a comment •

edited

Loading

ranile left a comment

ranile Nov 21, 2021

voidpumpkin Nov 21, 2021

voidpumpkin commented Nov 21, 2021

NickHu commented Dec 22, 2021

ranile commented Dec 22, 2021

Add basic lints to the HTML macro. #1748

Add basic lints to the HTML macro. #1748

Conversation

teymour-aldridge commented Feb 17, 2021 • edited Loading

teymour-aldridge commented Feb 17, 2021

github-actions bot commented Feb 17, 2021 • edited Loading

teymour-aldridge commented Feb 17, 2021

siku2 left a comment

Choose a reason for hiding this comment

siku2 commented Feb 18, 2021

teymour-aldridge commented Feb 18, 2021

teymour-aldridge commented Feb 18, 2021

siku2 left a comment

Choose a reason for hiding this comment

ranile left a comment

Choose a reason for hiding this comment

lukechu10 left a comment

Choose a reason for hiding this comment

lukechu10 Mar 3, 2021

Choose a reason for hiding this comment

teymour-aldridge commented Jun 25, 2021

mc1098 left a comment

Choose a reason for hiding this comment

mc1098 commented Sep 25, 2021

voidpumpkin commented Nov 20, 2021

mc1098 left a comment • edited Loading

Choose a reason for hiding this comment

ranile left a comment

Choose a reason for hiding this comment

ranile Nov 21, 2021

Choose a reason for hiding this comment

voidpumpkin Nov 21, 2021

Choose a reason for hiding this comment

voidpumpkin commented Nov 21, 2021

NickHu commented Dec 22, 2021

ranile commented Dec 22, 2021

teymour-aldridge commented Feb 17, 2021 •

edited

Loading

github-actions bot commented Feb 17, 2021 •

edited

Loading

mc1098 left a comment •

edited

Loading