#673 Fix HBS Lint Errors #675

BradenLawrence · 2020-10-16T12:40:21Z

Addressing linter errors found in hbs files following the latest standards of ember-template-lint: #673

ijlee2 · 2020-10-16T13:13:01Z

Hi, @BradenLawrence. I noticed that the CI (some of the tests) didn't pass. Will you check this?

Let's also work on updating your commit history. I looked at the 5 commits, and it seemed like it's the last commit that we want to keep and merge to the default branch? If so, try running the following commands:

# Set upstream
git remote set upstream [email protected]:ember-learn/ember-website.git

# Check if upstream was set correctly (you will see 2 lines for origin, and another 2 lines for upstream)
git remote -v

# Get the latest code and branches from the master branch
git checkout master
git fetch upstream
git merge upstream/master
git push origin master

# Install packages
npm install

# Rebase your branch
git checkout 673-Fix-HBS-Lint-Errors
git reset --hard HEAD~5
git pull --rebase origin master

# Restore your 5th (last) commit
git cherry-pick d197f69

BradenLawrence · 2020-10-16T13:32:33Z

Thanks for taking a look at the commit history! I'll try running that and then take a look at the test issues.

BradenLawrence · 2020-10-17T04:53:10Z

Hey @ijlee2! I took care of the Travis issues that came up. Thanks again for helping me manage my branches/commits! The only linter issues I wasn't sure how to fix were the "no-action" errors found on addon-tabs.hbs and ember-survey.hbs. Do you have any advice on how to tackle those?

.template-lintrc.js

ijlee2 · 2020-10-17T14:14:04Z

@BradenLawrence Thanks for addressing many of the HBS lint errors. They are not trivial to fix so I'm glad that I was able to get your help.

I think it's okay that we merge this pull request without fixing no-action lint errors. My near-term goal for this app is to Octanify it (update classic components to Glimmer and write more tests to help with maintenance). I imagine that, as a part of Glimmerizing components, some of the no-action errors can be fixed.

From the number of changed files, I think it may take me a day or more to complete the review. I'll let you know if I need additional changes. Hope you have a good weekend!

mansona

Hey 👋 thanks for the contribution!

I've noted a few issues, but I ran out of time to review this whole PR so I had to quit half way through 🙈 Is there any chance that we can split this up into a few smaller PRs? This is so that a) it's easier to review and b) we don't miss some important things (such as the change to nofollow) in the large changeset

Let me know if you want any help splitting this up, I already see that @ijlee2 has given you some git help 👍

app/components/ember-survey.js

mansona · 2020-10-18T21:53:22Z

app/templates/community/meetups/assets.hbs

@@ -74,7 +74,7 @@
      </h3>

      <p>
-        The Ember Core Team has a small stash of Ember stickers donated by the folks at <a href="https://www.tilde.io/" rel="nofollow noopener" target="_blank">Tilde</a>. These are distributed as requested while in stock. If you'd like to print your own Ember stickers, here are the files you'll need for the canonical ones.


why are we removing the nofollow from this link? changing anything that affects SEO should be a larger question and shouldn't be included in a "general fix" PR 👍

mansona · 2020-10-18T21:57:23Z

app/templates/components/homepage-image-grid.hbs

@@ -1,28 +1,28 @@
 <div class="homepage-image-grid" ...attributes>
  <div class="homepage-image-grid__scroll-wrapper">
    <figure class="homepage-image-grid__img-short">
-      <img class="homepage-image-grid__img" src="/images/community/tinified/EmberConf19-13.jpg" alt="Ember core team members gather around the Ember logo sign at EmberConf" loading="lazy">
+      <img class="homepage-image-grid__img" src="/images/community/tinified/EmberConf19-13.jpg" alt="" role="presentation" loading="lazy">


why are we removing the alt and adding presentation here? are they not navigable using the screen reader?

This image was flagged by Invalid alt attribute. Words such as image, photo,orpicture are already announced by screen readers. require-valid-alt-text due to the word "logo", and I noticed it played a similar role to the other images that were marked as presentation, i.e. being unrelated to the navigation of the page. I'm still pretty new to accessibility optimization though, so I'm happy to change it back (minus the word 'logo').

ijlee2 · 2020-10-20T21:39:43Z

@BradenLawrence Sorry for a long wait. I'm starting reviewing now.

ijlee2 · 2020-10-20T21:56:55Z

app/templates/community/meetups/index.hbs

-        {{#each model as |meetup|}}
-          {{#marker-layer lat=meetup.lat lng=meetup.lng}}
-            {{#popup-layer}}
+        {{#each this.model as |meetup|}}


suggestion (blocking):

Here, model came from the route's model hook so we want to use @model. (We can also guess this because the controller file doesn't exist.)

Suggested change

{{#each this.model as |meetup|}}

{{#each @model as |meetup|}}

ijlee2 · 2020-10-20T21:59:44Z

app/templates/ember-community-survey-2019.hbs

@@ -99,7 +99,7 @@
    <a href="#SS_Q104" id="SS_Q104" class="text-lg">
      How long have you been working with Ember?
    </a>
-    <HighCharts @chartOptions={{SS_Q104.options}} @content={{SS_Q104.data}} @theme={{theme}} />
+    <HighCharts @chartOptions={{SS_Q104.options}} @content={{SS_Q104.data}} @theme={{this.theme}} />


suggestion (non-blocking):

In lines 102, 175, and 258, we can add this.. (Not crucial since the code is commented out, but nice to do for consistency.)

Suggested change

<HighCharts @chartOptions={{SS_Q104.options}} @content={{SS_Q104.data}} @theme={{this.theme}} />

<HighCharts @chartOptions={{this.SS_Q104.options}} @content={{this.SS_Q104.data}} @theme={{this.theme}} />

ijlee2 · 2020-10-20T22:00:39Z

app/templates/head.hbs

praise:

Nice job with using this.model instead of @model. (I had inadvertently used the latter and caused a production bug.)

ijlee2 · 2020-10-20T22:02:49Z

app/templates/releases/beta.hbs

+    @projects={{array this.model.ember this.model.emberData}}
+    @description={{this.description}}


suggestion (blocking for line 6, soft-blocking for line 7):

In line 6, I think we want to pass @model, the data returned from the route's model hook.

I noticed that the <ProjectListing> component doesn't use the argument @description at all. Let's delete line 7.

Suggested change

@projects={{array this.model.ember this.model.emberData}}

@description={{this.description}}

@projects={{array @model.ember @model.emberData}}

ijlee2 · 2020-10-20T22:04:07Z

app/templates/releases/canary.hbs

-  channel="canary"}}
-    {{#terminal-code}}
+  <EmberCliUsage
+    @project={{this.model.ember}}


suggestion (blocking):

Suggested change

@project={{this.model.ember}}

@project={{@model.ember}}

ijlee2 · 2020-10-20T22:05:24Z

app/templates/releases/canary.hbs

-      npm install --save-dev https://s3.amazonaws.com/builds.emberjs.com{{model.canaryInfo.assetPath}}
-    {{/terminal-code}}
-  {{/ember-cli-usage}}
+      npm install --save-dev https://s3.amazonaws.com/builds.emberjs.com{{this.model.canaryInfo.assetPath}}


Suggested change

npm install --save-dev https://s3.amazonaws.com/builds.emberjs.com{{this.model.canaryInfo.assetPath}}

npm install --save-dev https://s3.amazonaws.com/builds.emberjs.com{{@model.canaryInfo.assetPath}}

ijlee2 · 2020-10-20T22:05:33Z

app/templates/releases/canary.hbs

-  project=model.emberData
-  channel="canary"}}
+  <EmberCliUsage
+    @project={{this.model.emberData}}


Suggested change

@project={{this.model.emberData}}

@project={{@model.emberData}}

ijlee2 · 2020-10-20T22:06:46Z

app/templates/releases/lts.hbs

-  channelDescription="Long Term Support releases receive bugfixes and security updates for an extended period."
-}}
+<ProjectListing
+  @projects={{array this.model}}


Suggested change

@projects={{array this.model}}

@projects={{array @model}}

ijlee2 · 2020-10-20T22:07:19Z

app/templates/releases/release.hbs

-  channelDescription="Releases are production-ready versions of Ember and Ember Data that have been through a six-week beta cycle."
-}}
+<ProjectListing
+  @projects={{array this.model.ember this.model.emberData}}


Suggested change

@projects={{array this.model.ember this.model.emberData}}

@projects={{array @model.ember @model.emberData}}

ijlee2

Hi, @BradenLawrence. I reviewed all files. Thank you again for working on fixing lint errors in templates.

✅ The vast majority of the changes looked great. I felt like you put in extra effort and care into improving accessibility (updating alt and tabindex), so I was glad and appreciative of your work. 💯

🙏🏼 It seemed to me like you (or a codemod) might have defaulted to using this.model. In Octane, we can write @model to indicate to a developer to look at the route file (versus the controller) if they ever need to know where model comes from. I made a few suggestions for using @model.

🙏🏼 With regards to removing nofollow, I would have liked to see you do this in a separate pull request. (If I'm understanding correctly, the removal wasn't related to fixing lint errors?)

I don't know if nofollow is relevant to SEO nowadays (SEO keeps changing so it's a mystery to me), but from a Search Console article from Google, it seemed like we do want to keep nofollow if we don't want the Ember website to be associated with the link (e.g. amazon.com). We probably do want to be associated with other Ember-related sites (i.e. do remove nofollow in such cases).

Because you made many removals of nofollow, I think a good approach to addressing @mansona's concern may be to keep the removals in this pull request, then make a separate pull request to bring back nofollow for links that we don't want to be associated with. Would this be okay with you?

I'm interested to hear why you removed nofollow so please feel free to give us feedback. If you have questions about my review or what the next steps can be, feel free to ask as well!

BradenLawrence · 2020-10-21T00:28:00Z

Hi @ijlee2 and @mansona, thank you both so much for the review! The @model vs this.model clarification is super helpful. For the nofollow links, I was replacing them with noopener noreferrer as suggested by this rule from the linter: error links with target="_blank" must have rel="noopener noreferrer" or rel="noreferrer noopener" link-rel-noopener, so no specific SEO motivation there. I'm happy to add them back in.

ijlee2 · 2020-10-21T14:07:36Z

@BradenLawrence Ah, I see. Yeah, I think the linter may have wanted us to add noopener noreferrer, but may not know what to do with nofollow.

If you have time to revert the nofollow changes, you are welcome to. Sorry for the mixup and extra time that you had to spend on your pull request. 😓

Braden incorporated Chris' feedback by reverting changes related to nofollow.

ijlee2

Awesome work, thank you! And congratulations on making your 1st contribution to the Ember website! 🎉

update hbs files to address linter errors

f0fad3f

BradenLawrence force-pushed the 673-Fix-HBS-Lint-Errors branch from d197f69 to f0fad3f Compare October 17, 2020 02:13

fix typo in sortedMeetupsByArea

0a2f6a8

remove log

4d5cc06

locks reviewed Oct 17, 2020

View reviewed changes

.template-lintrc.js Outdated Show resolved Hide resolved

ijlee2 self-requested a review October 17, 2020 14:10

BradenLawrence added 2 commits October 17, 2020 11:51

remove comments from linter rules

9bb4abc

disable no-action lint rule

40d69da

mansona previously requested changes Oct 18, 2020

View reviewed changes

ember-learn deleted a comment from mansona Oct 20, 2020

ijlee2 reviewed Oct 20, 2020

View reviewed changes

ijlee2 requested changes Oct 20, 2020

View reviewed changes

BradenLawrence added 3 commits October 21, 2020 17:48

revert removal of tagName

cc263fa

revert removal of 'nofollow' in rel properties

61563e3

fix this.model to @model

1b72836

ijlee2 approved these changes Oct 22, 2020

View reviewed changes

ijlee2 merged commit 7864f05 into ember-learn:master Oct 22, 2020

ijlee2 linked an issue Oct 22, 2020 that may be closed by this pull request

Fix HBS lint errors #673

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#673 Fix HBS Lint Errors #675

#673 Fix HBS Lint Errors #675

BradenLawrence commented Oct 16, 2020

ijlee2 commented Oct 16, 2020

BradenLawrence commented Oct 16, 2020

BradenLawrence commented Oct 17, 2020

ijlee2 commented Oct 17, 2020

mansona left a comment

mansona Oct 18, 2020

mansona Oct 18, 2020

BradenLawrence Oct 21, 2020

ijlee2 commented Oct 20, 2020

ijlee2 Oct 20, 2020

ijlee2 Oct 20, 2020

ijlee2 Oct 20, 2020

ijlee2 Oct 20, 2020 •

edited

Loading

ijlee2 Oct 20, 2020

ijlee2 Oct 20, 2020

ijlee2 Oct 20, 2020

ijlee2 Oct 20, 2020

ijlee2 Oct 20, 2020

ijlee2 left a comment •

edited

Loading

BradenLawrence commented Oct 21, 2020

ijlee2 commented Oct 21, 2020

ijlee2 left a comment

	{{#each this.model as \|meetup\|}}
	{{#each @model as \|meetup\|}}

	<HighCharts @chartOptions={{SS_Q104.options}} @content={{SS_Q104.data}} @theme={{this.theme}} />
	<HighCharts @chartOptions={{this.SS_Q104.options}} @content={{this.SS_Q104.data}} @theme={{this.theme}} />

		@projects={{array this.model.ember this.model.emberData}}
		@description={{this.description}}

	@projects={{array this.model.ember this.model.emberData}}
	@description={{this.description}}
	@projects={{array @model.ember @model.emberData}}

	npm install --save-dev https://s3.amazonaws.com/builds.emberjs.com{{this.model.canaryInfo.assetPath}}
	npm install --save-dev https://s3.amazonaws.com/builds.emberjs.com{{@model.canaryInfo.assetPath}}

	@project={{this.model.emberData}}
	@project={{@model.emberData}}

#673 Fix HBS Lint Errors #675

#673 Fix HBS Lint Errors #675

Conversation

BradenLawrence commented Oct 16, 2020

ijlee2 commented Oct 16, 2020

BradenLawrence commented Oct 16, 2020

BradenLawrence commented Oct 17, 2020

ijlee2 commented Oct 17, 2020

mansona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijlee2 commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijlee2 Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijlee2 left a comment • edited Loading

Choose a reason for hiding this comment

BradenLawrence commented Oct 21, 2020

ijlee2 commented Oct 21, 2020

ijlee2 left a comment

Choose a reason for hiding this comment

ijlee2 Oct 20, 2020 •

edited

Loading

ijlee2 left a comment •

edited

Loading