Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added fbjs eslint config, fixed lint errors and warnings #104

Merged
merged 2 commits into from
Oct 10, 2017

Conversation

jxom
Copy link
Contributor

@jxom jxom commented Oct 9, 2017

I decided to remove eslint:recommended config and just use fbjs to be consistent with the React project... Let me know if this is better!

@reactjs-bot
Copy link

reactjs-bot commented Oct 9, 2017

Deploy preview ready!

Built with commit 4b155a2

https://deploy-preview-104--reactjs.netlify.com

@bvaughn
Copy link
Contributor

bvaughn commented Oct 9, 2017

I decided to remove eslint:recommended config and just use fbjs to be consistent with the React project...

I think this is much better! 😄

@bvaughn bvaughn self-requested a review October 9, 2017 21:47
"eslint-plugin-prettier": "^2.3.1",
"eslint-plugin-react": "^7.4.0",
"eslint-plugin-relay": "^0.0.19",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to the package.json should also be accompanied by changes to yarn.lock (but I'll be happy to commit this myself after merging!)

}}>
}}
role="link"
tabIndex="-1">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm! It looks like MetaTitle is only clickable in one instance- inside of Section component. Technically we only need role="button" and tabIndex={0} in that case, and it's misattributed in the others (since it's not clickable).

What do you think about maybe removing onClick (and the role and tab index) from this component and moving that big into Section instead? Something like...

diff --git a/src/templates/components/MetaTitle/index.js b/src/templates/components/MetaTitle/index.js
index 165ad7b5b..6d02296aa 100644
--- a/src/templates/components/MetaTitle/index.js
+++ b/src/templates/components/MetaTitle/index.js
@@ -12,27 +12,17 @@
 import React from 'react';
 import {colors} from 'theme';
 
-const MetaTitle = ({
-  children,
-  title,
-  cssProps = {},
-  onClick,
-  onDark = false,
-}) => (
+const MetaTitle = ({children, title, cssProps = {}, onDark = false}) => (
   <div
-    onClick={onClick}
     css={{
       color: onDark ? colors.subtleOnDark : colors.subtle,
-      cursor: onClick ? 'pointer' : null,
       fontSize: 14,
       fontWeight: 700,
       lineHeight: 3,
       textTransform: 'uppercase',
       letterSpacing: '0.08em',
       ...cssProps,
-    }}
-    role="link"
-    tabIndex="-1">
+    }}>
     {children}
   </div>
 );
diff --git a/src/templates/components/Sidebar/Section.js b/src/templates/components/Sidebar/Section.js
index 517a89dd1..8f1044e82 100644
--- a/src/templates/components/Sidebar/Section.js
+++ b/src/templates/components/Sidebar/Section.js
@@ -26,32 +26,39 @@ const Section = ({
   section,
 }) => (
   <div>
-    <MetaTitle
+    <button
       onClick={onSectionTitleClick}
-      cssProps={{
-        marginTop: 10,
-
-        [media.greaterThan('small')]: {
-          color: isActive ? colors.text : colors.subtle,
-
-          ':hover': {
-            color: colors.text,
-          },
-        },
+      css={{
+        appearance: 'none',
+        background: 'transparent',
+        border: 0,
       }}>
-      {section.title}
-      <ChevronSvg
+      <MetaTitle
         cssProps={{
-          marginLeft: 7,
-          transform: isActive ? 'rotateX(180deg)' : 'rotateX(0deg)',
-          transition: 'transform 0.2s ease',
+          marginTop: 10,
 
-          [media.lessThan('small')]: {
-            display: 'none',
+          [media.greaterThan('small')]: {
+            color: isActive ? colors.text : colors.subtle,
+
+            ':hover': {
+              color: colors.text,
+            },
           },
-        }}
-      />
-    </MetaTitle>
+        }}>
+        {section.title}
+        <ChevronSvg
+          cssProps={{
+            marginLeft: 7,
+            transform: isActive ? 'rotateX(180deg)' : 'rotateX(0deg)',
+            transition: 'transform 0.2s ease',
+
+            [media.lessThan('small')]: {
+              display: 'none',
+            },
+          }}
+        />
+      </MetaTitle>
+    </button>
     <ul
       css={{
         marginBottom: 10,

onClick={this._openNavMenu}>
onClick={this._openNavMenu}
role="button"
tabIndex="-1">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be tabIndex={0} so it's keyboard accessible.

@jxom
Copy link
Contributor Author

jxom commented Oct 9, 2017

Sounds great, @bvaughn! Will make those changes.

@jxom
Copy link
Contributor Author

jxom commented Oct 10, 2017

@bvaughn - Seems like the button produces a blue outline when clicked (:focus). Removing it with outline: 'none' will also encounter accessibility issues... What do you reckon should we do? Leave it, or remove the outline completely, or add another style (such as a dark text color) for focus?

@bvaughn
Copy link
Contributor

bvaughn commented Oct 10, 2017

Yup. The outline is important for keyboard users until/unless we provide a different focus style (cc @joecritch). I think for now we should leave it in.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Thanks!

@bvaughn bvaughn merged commit 0eb0110 into reactjs:master Oct 10, 2017
@jxom
Copy link
Contributor Author

jxom commented Oct 10, 2017

Awesome!

@jxom jxom deleted the create-script-for-checks-in-ci branch October 10, 2017 23:44
jhonmike pushed a commit to jhonmike/reactjs.org that referenced this pull request Jul 1, 2020
BetterZxx pushed a commit to BetterZxx/react.dev that referenced this pull request Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants