Test plan:
- Enable the Notes column
- Add a note to a student
- Verify that the note is displayed after saving it
- Edit the note
- Verify that the note is updated after saving it
- Clear the note
- Verify that the note is cleared after saving it
- Disable the Notes column
- Verify that the notes column is removed from the UI
- Re-enable the Notes column
- Make a change to a note
- Verify that the note is displayed after refreshing the page
- Verify that notes column loads properly in a course with many
students
Closes EVAL-1934
flag=none
Change-Id: I2245c08e1f4b766156febbbc791acdd7c4e4c73f
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/306852
Reviewed-by: Cameron Ray <cameron.ray@instructure.com>
Reviewed-by: Kai Bjorkman <kbjorkman@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Kai Bjorkman <kbjorkman@instructure.com>
Product-Review: Cameron Ray <cameron.ray@instructure.com>
reimplement enhanceUserContent without
jquery in preparation for moving out of the
canvas monolith. Some transformations to the DOM
aren't necessary, but are there for backward compatibility
(as I discovered getting all existing specs to pass) pass
This is not going to be an easy review, since the new implementation
is completely new and it will be difficult to ensure the
previous functionality is all implemented correctly.
What can I say?
closes MAT-1011
flag=none
test plan:
- using the rce, create content with
- external link
- course link
- canvas file link, default overlay preview
- canvas file link, inline preview
- canvas file link, inline preview, automatially expand
- link to youtube content, end edit html and remove
"inline_disabled" class name
- save
> expect
> external link to have the external link icon
w/in the link so you can click it and open
the link in a new tab
> nothing happens to the course link
> clicking canvas file link 1, preview opens in the overlay
> clicking canvas file link 2, preview opens inline
> clicking again closes preview
> canvas file link 3 inline preview opens automatically
> clicking link closes preview
> the youtube link gets a preview thumbnail next to it
> click the thumbnail and it's replaced with a playable
youtube video
- have a student submit an assignment
- create a media comment on the assignment
> expect the media comment link to be replaced with a thumbnail
that plays when clicked (I think)
- PS29 and beyone
- look at the DOM tree for external images
> expect the text and icon each to be in their own span
- have a link containing just an image.
- tab through the saved content
> expect the linked image's focus ring to be in your theme color
TESTING JQUERY WIDGETS: this only works in canvas, probably not new quizzes
- put something like the following in your content using the rce's html editor
<div class="enhanceable_content draggable" style="width: 100px; height: 100px; border: 1px solid black; background-color: cyan;">drag me?</div>
<div class="enhanceable_content resizable" style="width: 100px; height: 100px; border: 1px solid black; background-color: grey;">resize me?</div>
<div class="enhanceable_content tabs">
<ul>
<li><a href="#tabs-1">Nunc tincidunt</a></li>
<li><a href="#tabs-2">Proin dolor</a></li>
<li><a href="#tabs-3">Aenean lacinia</a></li>
</ul>
<div id="tabs-1">
<p>I am tab 1 content</p>
</div>
<div id="tabs-2">
<p>This is tab 2 content</p>
</div>
<div id="tabs-3">
<p>tab 3 content has 2 paragraphs</p>
<p>and this is that 2nd paragraph</p>
</div>
</div>
<p><a href="#jquery_dialog">open the dialog</a></p>
<div id="jquery_dialog" class="enhanceable_content dialog" title="Basic dialog">
<p>This is the default dialog which is useful for displaying information. The dialog window can be moved, resized and closed with the 'x' icon.</p>
</div>
<div>Drag items around to sort</div>
<ul class="enhanceable_content sortable">
<li>Item 1</li>
<li>Item 2</li>
<li>Item 3</li>
</ul>
> expect to be able to drag "drag me" around the page
> expect "resize me" to be resizeaable
> expect tabs to work
- click "open the doalog"
> expect the dialog to open
> expect to be able to close and reopen the dialog
> expect to be able to drag list items around to reorder them
Change-Id: I34418970870a1d34f8dabd9dd435ba058576b52a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/300806
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Mysti Lilla <mysti@instructure.com>
QA-Review: Mysti Lilla <mysti@instructure.com>
Product-Review: Ed Schiebel <eschiebel@instructure.com>
Refs FOO-3064
flag=none
React recommends always passing a value to a Boolean prop
even if it is true, ie `isSelected={true}` instead of just
`isSelected`
The reason given in the React guides is to improve JSX readability.
Test plan:
* none
Change-Id: I7159638ac8deec86752f6d8c38ecaee4743c3302
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/299260
Reviewed-by: Sean Scally <sean.scally@instructure.com>
QA-Review: Charley Kline <ckline@instructure.com>
Product-Review: Charley Kline <ckline@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Refs FOO-2801
flag=none
The various builds look at various filesystem trees so
the ESlint config isn't consistent. A previous commit
passed the pre-merge build fine but failed the post-merge
job. This should fix that.
Test plan:
* Pre-merge build is still happy
* Post-merge build is happier still
Change-Id: Ic06225fcd8a82deb8f819acd27f557512bc15776
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/298424
QA-Review: Charley Kline <ckline@instructure.com>
Product-Review: Charley Kline <ckline@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Andrea Cirulli <andrea.cirulli@instructure.com>
refs FOO-2697
flag = none
those two packages were relying on canvas's babel.config.js but no more
test plan: the build phase of the CI is enough as there were no logical
changes
Change-Id: I7814796f36f1e30b4dd494a12f768f9dbafb4f1f
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/286759
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: August Thornton <august@instructure.com>
Product-Review: August Thornton <august@instructure.com>
QA-Review: Ahmad Amireh <ahmad@instructure.com>
minor QOL mainly for me; put webpack-specific configuration in its own
folder, make it portable, and make room for esbuild
the __webpack_public_path__ initializer no longer sources the value from
the build module, instead it uses a pre-defined global that webpack
injects
~ test plan ~
build is OK
Change-Id: I4ba4a3c0cb9175f96096f2b78022e152c04fc75d
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/276183
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Charley Kline <ckline@instructure.com>
QA-Review: Charley Kline <ckline@instructure.com>
Product-Review: Charley Kline <ckline@instructure.com>
flag=none
With this linter, we will be unable to skip any tests for any reason
(ex flakiness). Since an eslint warn is still considered a -2, we will
disable this rule for now.
test plan:
- Run `node_modules/.bin/eslint` on a test file with a test skipped
- the linter should not flag the skipped test
qa risk: low
Change-Id: I5dbeef818ee550f10b4fa8352bbca20b47b1f7f0
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/274670
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Kyle Rosenbaum <krosenbaum@instructure.com>
QA-Review: Davis Hyer <dhyer@instructure.com>
Product-Review: Davis Hyer <dhyer@instructure.com>
This change brings in the basic pace plans frontend with
styled-components and several other smaller libraries replaced with
InstUI 7 components. It also adds the 'reselect' library as a direct
dependency (which we already had through @instructure/outcomes-ui) and
'tsc-files' for type-checking of staged TS files on commit. There were
also some tweaks to typescript and eslint configs, mostly to get both
up to speed with the typescript code.
Finally, this also adds a `pace_plans` endpoint to
`courses_controller` to bootstrap the frontend-- this will get moved
to `pace_plans_controller` once it has been merged.
It's also worth noting that no frontend tests are included with this
change-- the existing tests were written with enzyme and are heavily
snapshot-based, so we will be replacing those with
@testing-library/react tests in later updates (in keeping with current
testing best practices at Instructure).
closes LS-2431, LS-2432, LS-2433, LS-2434, LS-2452
flag = pace_plans
Test plan:
- Set up a course with at least one module and several module items
- Turn on the pace_plans feature flag in the account associated with
that course
- Turn on the "Enable pace plans" setting in course settings
- Create a pace plan for the course via the Rails console:
c = Course.find<id>
pp = c.pace_plans.create! workflow_state: 'active'
c.context_module_tags.each_with_index do |t, i|
pp.pace_plan_module_items.create! module_item: t, duration: i*2
end
- Go to the course as a teacher or admin
- Expect to see a "Pace Plans" link in the course navigation
- Click it, expect the pace plan you created earlier to load and
render
- Expect to be able to pick dates, change durations, and toggle
checkboxes (although saves will fail, since there is no API yet).
- Expect to not see the "Pace Plans" course nav link when the feature
flag or course setting is off
- Expect /courses/<id>/pace_plans to return a 404 when the feature
flag or course setting is off
- Expect to not see the "Pace Plans" course nav link as a student
- Expect /courses/<id>/pace_plans to display an "Unauthorized" page
as a student
Change-Id: If4dc5d17f2c6a2109d4b4cb652c9e9ef00d7cc33
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271650
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ed Schiebel <eschiebel@instructure.com>
Product-Review: Jeff Largent <jeff.largent@instructure.com>
This change adds support for typescript transpilation to our frontend
build via babel, and also updates eslint and canvas_i18nliner to be
able to understand typescript as well. The main goal of this PS is to
enable developers to add typescript code to the Canvas codebase but to
be unopinionated about how type-checking is done; at this stage types
will only be checked by running the new `check:ts` or `check:js`
scripts (which run the typescript compiler directly), or via
integration with an IDE like RubyMine or VS Code.
closes LS-2430
flag = none
Test plan:
BUILD STEPS:
- FE build, i18n build, and tests pass Jenkins
- `bin/rails canvas:compile_assets` still works
- `RAILS_ENV=production bin/rails convas:compile_assets` still works
SPOT CHECKING:
- Starting up rails and run `yarn build:js:watch`
- Click around Canvas and make sure the frontend still loads as
normal
Change-Id: I8bb1a0f065e09496a924708dead6fa4518b59496
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/270401
Reviewed-by: Ahmad Amireh <ahmad@instructure.com>
Reviewed-by: Nate Armstrong <narmstrong@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
QA-Review: Ahmad Amireh <ahmad@instructure.com>
Product-Review: Jeff Largent <jeff.largent@instructure.com>
so it stops complaining about the ?. operator, among other things
closes MAT-45
flag=none
test plan:
- npx eslint ./path/to/a.js
on a file with ?. in it doesn't complain
Change-Id: I6056e25ce9c53a8e652d6e4eec1280057369cf26
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/265387
Reviewed-by: Guilherme Baron <gbaron@instructure.com>
QA-Review: Guilherme Baron <gbaron@instructure.com>
Product-Review: Ed Schiebel <eschiebel@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
test plan:
- no more linting failures when file committed
without trailing commas
- yet prettier implements new default behavior
flag=none
Change-Id: I3884e5f512f06dea2c08c6fecb8573aed3a7798e
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/258196
Reviewed-by: Spencer Olson <solson@instructure.com>
Reviewed-by: Rob Orton <rob@instructure.com>
Reviewed-by: Matthew Lemon <mlemon@instructure.com>
QA-Review: Rob Orton <rob@instructure.com>
Product-Review: Rob Orton <rob@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
fixes FOO-1409
flag = none
no more client_apps, canvas_quizzes now lives as part of canvas-lms
proper inside app/jsx/, which makes the build leaner and leaves us with
one less thing to reason about
logical changes:
- converted from AMD to ES modules
- upgraded to recent react + react-router
- dropped RSVP in favor of native Promises
- used CanvasModal instead of home-grown Dialog
- removed dead code; notifications in particular were fishy as there had
no dependents at all and did not even show up in the graph
- ported tests to Jest, added more unit ones and two integration ones
- removed "config.onError" and now throws errors where appropriate
- disabled console statements in non-dev
:: test plan ::
- create a (old-school) quiz containing all types of questions
- as 3 distinct students, take the quiz and try to randomize your
answers
at this point it's helpful to have a reference to compare the screens; I
replicated the quiz on my production sandbox for this
- go to /courses/:id/quizzes/:id/submissions/:id/log
- verify it looks OK
- click on a specific question in the stream and verify the question
inspector widget works OK
- go back to stream and push "View table"
- verify the table and its controls are OK
- go to /courses/:id/quizzes/:id/statistics
- verify it looks OK
- click on ? in the discrimination index chart and verify it displays
a dialog with help content
- click on "X respondents" in one of the charts and verify it displays
a dialog with the respondent names
- verify the interactive charts do interact as expected (no logic
changed here so just a quick glance)
- link to "View in SpeedGrader" for essay-like questions works
Change-Id: I79af5ff4f1479503b5e2528b613255dde5bc45d3
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/256118
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
closes LA-995
flag=none
1. unify the instui ui-babel-preset options across sub-packages
2. include "@instructure" as part of each sub-package name
3. unify the commonjs and es module directory structure
4. use index.js to export anything needed to be imported from
outside the package. This removes any dependency on the
package's internal directory structure and makes importing
independent of commonjs vs es module simple
5. move canvas-rce's main entry from async.js to index.js
(I'm 98% sure index.js was an artifact of the old rce)
test plan:
- canvas-lms builds locally and in docker. (Jenkins builds
the docker image, so I presume if it passes jenkins,
the docker build wasn't broken)
- sniff test the result, esp. student planner and the new rce.
- yarn build:watch from canvas-rce, canvas-planner, and
canvas-media packages should work, and yarn build:watch
from canvas-lms should pick-up any changes.
(I've verified this by finding a console.log statement
and changing the message then use devtools to see it's in
the new source after refreshing the browser)
Change-Id: I5f242d06ad655597cd416057d4b740d4bd86003b
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/238603
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Alex Anderson <raanderson@instructure.com>
QA-Review: Robin Kuss <rkuss@instructure.com>
Product-Review: Ed Schiebel <eschiebel@instructure.com>
This makes it so all staged *.js files are formatted using
prettier as a precommit hook rather than just selected
portions.
Test Plan:
- Commit a JS file with something not pretty in it.
- It should be pretty after the commit.
closes COREFE-347
closes COREFE-50
flag = none
Change-Id: Id9ad77e68fd95720752a316e5f2111ab6c9a02bc
Reviewed-on: https://gerrit.instructure.com/212895
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
We agree that people _should_ not use PropTypes.object normally, but
there _are_ times where doing that makes more sense then trying to come
up with a PropTypes.shape to use instead. So this is something we think
should be enforced in code-review rather than -2ing commits because
of it. We’d rather get rid of the rule than see a bunch of
`eslint-disable-next-line react/forbid-prop-types` scattered around
trying to ignore it where people knew what they were doing.
Test plan:
* you shouldn’t see any more warnings about
`PropTypes.object is forbidden` when you eslint
Change-Id: Icc9071604a162260aa9bce3a916f046f2553b66a
Reviewed-on: https://gerrit.instructure.com/209451
Tested-by: Jenkins
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
this disallows things like creating 2 react classes in one file
like we do in app/jsx/shared/TermsOfServiceModal.js
I don’t see how that is going to catch any bugs and I don’t see why
we should tell people they can’t do that so I vote turn this off
test plan:
if you agree +2, if not, that’s fine and I’ll abandon
Change-Id: I0751609a75051e379f958aaed2e1c4dd260c020d
Reviewed-on: https://gerrit.instructure.com/207306
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
test plan:
- node ./node_modules/.bin/eslint packages/canvas-rce/src/bridge/Bridge.js
does not generate a warning or error.
Change-Id: I136f9bce39ea9a85999cbd5b244fd9d021b3a123
Reviewed-on: https://gerrit.instructure.com/205915
Reviewed-by: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
The previous commit makes it so i18nliner can understand <> instead of
<React.Fragment>, so this turns back on the eslint rule we had to turn
Off
Test plan:
* run bin/rake i18n:generate_js (or let the Jenkins builds run)
* it should pass
Change-Id: Ia3204cd6c3147eb52ca612adbb3b20b2df8c6921
Reviewed-on: https://gerrit.instructure.com/205378
Tested-by: Jenkins
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
I wouldn't mind this rule, but it tries to force you into syntax that
i18nliner can't support.
Change-Id: Id6238223850ec134dd9aea1ddf34e1713f288e39
Reviewed-on: https://gerrit.instructure.com/205307
Reviewed-by: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
instead of having our own eslint in canvas-rce, and maintaining all
future rules like react hooks and stuff we can just use the same one
that the rest of the repo uses.
test plan:
* cd packages/canvas-rce
* `yarn lint` should run
Change-Id: Idbe8af533f7a19035d1998538d2d6fd1a711f164
Reviewed-on: https://gerrit.instructure.com/204998
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
AirBnB’s styleguide wants to force you to write
“foo” + bar
as
`foo${bar}`
see: https://eslint.org/docs/rules/prefer-template
I agree that the former is probably easier to read but I don’t know that
I care that much about to _force_ it. Sometimes I see things that use
“+” that seem nicer to read than the ones that use template literals.
and I just don’t think this is going to catch any bugs either way.
what say ye?
- 1 this if you want to keep caring about it
+2 this if you agree we should stop caring
Change-Id: Ie2fd6267dd0566da73034f7a62bd9dd73ebf4138
Reviewed-on: https://gerrit.instructure.com/205253
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
this commit increases the error-level on prop types from warning to
error.
it doesn't alert on components that don't have proptypes declared.
Change-Id: I6f74f1f6f3b3e0867002199e5507466038d8e1c3
Reviewed-on: https://gerrit.instructure.com/204773
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
We frequently have code like:
const {id, children} = props
const otherProps = omitProps(props, null, ['id', 'value', 'onChange'])
you could rewrite that as:
const {id, children, value, onChange, ...otherProps} = this.props
But if you did that right now, without this change, you’d get an eslint
error about `no-unused-vars` because you didn’t use value or onChange
in this function.
This makes it so eslint doesn’t complain about that.
See: https://eslint.org/docs/rules/no-unused-vars#ignorerestsiblings
Test plan:
* So the question is:
* Do you think that complaining about this rule actually catches any
real bugs?
* if so, -2 this.
* if not, +2 it
Change-Id: I2fc9b351f34e80ec74199569f768ccc71dfcd705
Reviewed-on: https://gerrit.instructure.com/202669
Tested-by: Jenkins
Reviewed-by: Ed Schiebel <eschiebel@instructure.com>
QA-Review: Ed Schiebel <eschiebel@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Update the react dependency to 16.8 since we're starting to use react
hooks, which requires that version at minimum.
test plan:
- linting still works
- intentionally create a violation of rules-of-hooks. Linting should
find and report the error.
Change-Id: I0baccd64397b31118b96eb390462acf56daeca96
Reviewed-on: https://gerrit.instructure.com/199053
Reviewed-by: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
…instead, just use the default config for the rest of the repo
Test plan:
* cd packages/canvas-planner
* yarn eslint src/components/BadgeList/index.js
* you should not see an error about not being able to find flowtype
Change-Id: I48c4ab3ac20a60352c20c489addb4d4b4a32e83a
Reviewed-on: https://gerrit.instructure.com/199060
Reviewed-by: Jon Willesen <jonw+gerrit@instructure.com>
QA-Review: Jon Willesen <jonw+gerrit@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Tested-by: Jenkins
This only affects javascript files and only affects things
from the prettier whitelist
This will run slowly if you don't have node_modules installed
locally (e.g., in Docker), but it will gladly attempt to
run things in Docker for you.
This adds a new githook_installer image that will install
the githook whenever a docker-compose up happens in the
repo. It will also install the hook whenever a `yarn`
occurs locally (as a postinstall hook).
This commit should also not fail things. For example
having unused variables is an ESLint error, but it isn't
autofixable. It will log the error, but will otherwise
continue. However, it will make this pretty with prettier
as well as fix any other autofixable ESLint errors.
closes CORE-2118
Test Plan:
- Run `yarn`
- Add some semicolons to something from the whitelist
- git add that file
- git commit and it will strip semicolons
- In a dockerized Canvas:
- docker-compose up
- Add semicolons to a file
- git add that file
- git commit, it will take forever (~60s)
- It should have stripped out semicolons
Change-Id: Id9198aa008808e898f29acb9ed64dd14ff843222
Reviewed-on: https://gerrit.instructure.com/171510
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Brent Burgoyne <bburgoyne@instructure.com>
Product-Review: Brent Burgoyne <bburgoyne@instructure.com>
I guess since our team owns these we can opt them in now
Test plan:
* nothing should change
Change-Id: Ic7207e1033869ef60644c41bd5c80a3e8532a6dd
Reviewed-on: https://gerrit.instructure.com/171471
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This commit introduces a better more reasonable ESLint configuration.
It removes all concerns with styling in favor of using prettier to handle
those things.
closes CORE-2096
Test Plan:
- ESLint works
Change-Id: I50b90b7191b576bce4817d885d14f18c4c72d208
Reviewed-on: https://gerrit.instructure.com/170874
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
We do this often enough in our actual code that I don’t think it is
worth the noise we create complaining about it. I feel it conditions
people to ignore other stuff that does matter.
I also feel it is a valid thing to do and forcing people to use new
variable names just to appease this rule is not helpful.
Here’s the docs about why the rule exists:
https://eslint.org/docs/rules/no-param-reassign
What say ye’all? Can we axe this?
Change-Id: Ib9e3c54e5a02d3bbc892d875448d71ae87b5e6c1
Reviewed-on: https://gerrit.instructure.com/167894
Tested-by: Jenkins
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
this is only needed to protect yourself in hostile environments
where someone might do something like Object.prototype.foobar = “blah”
Because we control all the javascript that is executed on our page,
That won’t ever happen. And if it did, there are a million and a half
other places in our code that will blow up before your code blows up.
Besides, we already catch that ^ in `no-extend-native`
So complaining to you and making you handle this case is just not needed
To read the docs of why this rule exists go here:
https://eslint.org/docs/rules/guard-for-in
What say ye’all? can we axe this?
Change-Id: Ie942578e0b8a3cce8130412f866556da7b9e984c
Reviewed-on: https://gerrit.instructure.com/167896
Tested-by: Jenkins
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
This rule is just there to protect against a pretty esoteric edge
case that we never do. So in effect, in our code, we never will run
into the case where you’ll need it
Read here for why you would need it:
https://eslint.org/docs/rules/no-prototype-builtins
What say ye’all can we axe this?
Change-Id: I4b9124dd457e843e8429eb701e1ddc77fc86c182
Reviewed-on: https://gerrit.instructure.com/167895
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This makes it so everything in app/jsx/account_course_user_search
conforms to our style guide (prettier)
Change-Id: I797121e1002eb1a5bcd28f6a177db3b13ae029eb
Reviewed-on: https://gerrit.instructure.com/159747
Tested-by: Jenkins
Reviewed-by: Brent Burgoyne <bburgoyne@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Test Plan:
- Run ./node_modules/.bin/eslint "app/jsx/**/*.*" | grep 'jsx-a11y'
- Unfortunately you should see some errors :(
Change-Id: I5de6b15471393a7efe5a324b7b55347b7eb34311
Reviewed-on: https://gerrit.instructure.com/155918
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Product-Review: Clay Diffrient <cdiffrient@instructure.com>
QA-Review: Clay Diffrient <cdiffrient@instructure.com>
These don’t objectively catch any bugs. They are just style preferences.
if people really want to do what they complain about, let’s assume they
know what they are doing and not bug them.
Eg: If they think it would be more readable to un-nest their ternary,
great. But if not, like if they are rendering a bunch of jsx lets not
make them make even more verbose and ugly code to not get “no-nested-
Ternaries” errors
And
<Foo ref={e => this.someRef = e} /> should be just fine. It doesn’t
help anything for people to make that more verbose
Change-Id: Ifa3461d8118038a20b42f568f682de3bae9f1f31
Reviewed-on: https://gerrit.instructure.com/153610
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Right now, eslint will tell you your imports should look like:
import bar from ‘compiled/foo/bar’
import {map} from ‘loadsh’
import baz from ‘../baz’
Once we get rid of all the “jsx/*” and “compiled/*” stuff from our
import paths and just use relative paths it will tell you to do:
import {map} from ‘loadsh’
import bar from ‘../../foo/bar’
import baz from ‘../baz’
So it doesn’t make sense to complain about this now and tell people
To put those compiled/foo paths up with the stuff from NPM and not with
the rest of the local stuff.
Note: it _is_ still encouraged to put all your imports in this order:
“nodeJS builtin (eg: import {readFile} from ‘fs’)”
"external (from node_modules)”
“local/relative (eg: ‘compiled/foo’ or ‘../bar’)”
There’s just not an eslint rule that can enforce that for us right now.
Change-Id: I83b0009882d525e9b999b8f9f5991361ce7981fa
Reviewed-on: https://gerrit.instructure.com/151102
Tested-by: Jenkins
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
if you set your editor to eslint --fix on save, which is already safe
be doing, it will also pass anything in this whitelist of dirs to
Opt-in to be prettier formatted too. So you don’t have to wait until
we prettier all the things later and can have consistency among your
now
Test plan:
* gergich should complain if something in app/jsx/permissions is not
Prettier formatted
Change-Id: I438d11f25b10ed58e53656e110aa0d1e7fed5660
Reviewed-on: https://gerrit.instructure.com/149583
Tested-by: Jenkins
Reviewed-by: Venk Natarajan <vnatarajan@instructure.com>
QA-Review: Steven Burnett <sburnett@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
these were all things gergich gives warnings for that everyone ignores
anyway. it is better to turn silly stuff off than to train people to
ignore gergich warnings
Change-Id: I0bc1f807c8c44c0fe6b180126cf8f6e9d14b1797
Reviewed-on: https://gerrit.instructure.com/147815
Reviewed-by: Clay Diffrient <cdiffrient@instructure.com>
Tested-by: Jenkins
Product-Review: Ryan Shaw <ryan@instructure.com>
QA-Review: Ryan Shaw <ryan@instructure.com>
Aaron Shafovaloff