Skip to content

Update Profile TrackSwitcher Icon onChange #830

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

Merged
merged 2 commits into from
Apr 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module.exports = {
'plugin:react-hooks/recommended',
'plugin:@typescript-eslint/eslint-recommended',
'plugin:@typescript-eslint/recommended',
'plugin:testing-library/react',
'prettier',
'prettier/@typescript-eslint',
],
Expand All @@ -21,11 +22,32 @@ module.exports = {
ecmaVersion: 12,
sourceType: 'module',
},
plugins: ['jest', 'react', '@typescript-eslint'],
plugins: ['jest', 'react', '@typescript-eslint', 'testing-library'],
rules: {},
settings: {
react: {
version: 'detect',
},
},
overrides: [
{
files: ['*.ts', '*.tsx'],
rules: {
'react/prop-types': 'off',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

prop-types aren't used in typescript. I think this is raising an error just because we have a mix of js and ts in the project

},
},
{
files: ['*.test.ts', '*.test.tsx'],
rules: {
'@typescript-eslint/no-empty-function': 'off',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is sometimes easy to use an empty function for testing without creating a complex requirement

'@typescript-eslint/no-unused-vars': 'off',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just my opnion here but in test files I don't think this matters too much. Maybe downgrading it to a warning rather than an error?

},
},
{
files: ['*.test.tsx'],
rules: {
'jest/expect-expect': 'off', // testing-library's getBy___ queries are in themselves assertions
},
},
],
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export const TrackSwitcher = ({
aria-label="Open the track switcher"
{...buttonAttributes}
>
<GraphicalIcon icon="all-tracks" />
<TrackLogo track={value} />
<div className="track-title">{value.title}</div>
<div className="count">
{getTotalReputation(value).toLocaleString()} rep
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
"eslint-plugin-jest": "^24.1.0",
"eslint-plugin-react": "^7.21.3",
"eslint-plugin-react-hooks": "^4.1.2",
"eslint-plugin-testing-library": "^4.1.2",
"flush-promises": "^1.0.2",
"fork-ts-checker-webpack-plugin": "^5.2.0",
"husky": "^4.2.5",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import * as React from 'react'

import { render, fireEvent, act, screen, within } from '@testing-library/react'
import '@testing-library/jest-dom/extend-expect'

import { TrackSwitcher } from '../../../../../app/javascript/components/profile/contributions-summary/TrackSwitcher'

import {
createTrack,
totalTrackReputation,
} from '../../../factories/ContributionSummaryFactory'

const setup = () => {
const tracks = [
createTrack(),
createTrack({
id: 'elixir',
title: 'Elixir',
iconUrl: '/icons/elixir.svg',
}),
]

return {
tracks,
}
}

describe('TrackSwitcher', () => {
test('shows value on render', () => {
const { tracks } = setup()

const currentTrack = tracks[0]

render(
<TrackSwitcher tracks={tracks} value={currentTrack} setValue={() => {}} />
)

screen.getByAltText(`icon for ${currentTrack.title} track`)
screen.getByText(currentTrack.title)
screen.getByText(`${totalTrackReputation(currentTrack)} rep`)
})

test('shows dropdown on click', async () => {
const { tracks } = setup()

const currentTrack = tracks[0]

render(
<TrackSwitcher tracks={tracks} value={currentTrack} setValue={() => {}} />
)

const button = screen.getByLabelText('Open the track switcher')

await act(async () => {
await fireEvent.click(button)
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually use userEvent.click instead of fireEvent.click. Is there any advantage to use fireEvent instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use fireEvent because that's what I know, but looking for a reference to think points out that using userEvent might be a better way to go about this. Reference: testing-library/eslint-plugin-testing-library#162 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@neenjaw Want to change or should I merge? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are essentially the same thing, so this works and covers it, but if I'm in this piece of code again I would change it. I think good to merge

})

const menu = screen.getByRole('menu')
const menuItems = within(menu).queryAllByRole('menuitem')
expect(menuItems).toHaveLength(tracks.length)
menuItems.forEach((menuItem, index) => {
within(menuItem).getByAltText(`icon for ${tracks[index].title} track`)
within(menuItem).getByText(tracks[index].title)
})
})
})
66 changes: 66 additions & 0 deletions test/javascript/factories/ContributionSummaryFactory.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { RepresenterFeedback } from '../../../app/javascript/components/mentoring/session/RepresenterFeedback'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice :) I should do something like this from now on.

import {
Category,
CategoryId,
Track,
} from '../../../app/javascript/components/profile/ContributionsSummary'

export const createCategoryId = (categoryId?: CategoryId): CategoryId =>
categoryId ? categoryId : 'publishing'

export const createCategory = (category?: Partial<Category>): Category => ({
id: createCategoryId(),
reputation: 140,
metricFull: '1 solution published',
metricShort: '1 solution',
...category,
})

export const createTrack = (track?: Partial<Track>): Track => ({
id: 'javascript',
title: 'JavaScript',
iconUrl: '/icons/javascript.svg',
categories: [
{
id: 'publishing',
metricFull: 'No solutions published',
metricShort: 'No solutions',
reputation: 0,
},
{
id: 'mentoring',
metricFull: 'No students mentored',
metricShort: 'No students',
reputation: 0,
},
{
id: 'authoring',
metricFull: '3 exercises contributed',
metricShort: '3 exercises',
reputation: 50,
},
{
id: 'building',
metricFull: '2 PRs accepted',
metricShort: '2 PRs accepted',
reputation: 24,
},
{
id: 'maintaining',
metricFull: '6 PRs reviewed',
metricShort: '6 PRs reviewed',
reputation: 35,
},
{
id: 'other',
reputation: 0,
},
],
...track,
})

export const totalTrackReputation = (track: Track): number => {
return track.categories.reduce((sum: number, category: Category) => {
return sum + category.reputation
}, 0)
}
53 changes: 53 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1784,6 +1784,18 @@
eslint-scope "^5.0.0"
eslint-utils "^2.0.0"

"@typescript-eslint/experimental-utils@^4.21.0":
version "4.22.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/experimental-utils/-/experimental-utils-4.22.0.tgz#68765167cca531178e7b650a53456e6e0bef3b1f"
integrity sha512-xJXHHl6TuAxB5AWiVrGhvbGL8/hbiCQ8FiWwObO3r0fnvBdrbWEDy1hlvGQOAWc6qsCWuWMKdVWlLAEMpxnddg==
dependencies:
"@types/json-schema" "^7.0.3"
"@typescript-eslint/scope-manager" "4.22.0"
"@typescript-eslint/types" "4.22.0"
"@typescript-eslint/typescript-estree" "4.22.0"
eslint-scope "^5.0.0"
eslint-utils "^2.0.0"

"@typescript-eslint/parser@^4.4.0":
version "4.4.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/parser/-/parser-4.4.0.tgz"
Expand All @@ -1794,6 +1806,14 @@
"@typescript-eslint/typescript-estree" "4.4.0"
debug "^4.1.1"

"@typescript-eslint/scope-manager@4.22.0":
version "4.22.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/scope-manager/-/scope-manager-4.22.0.tgz#ed411545e61161a8d702e703a4b7d96ec065b09a"
integrity sha512-OcCO7LTdk6ukawUM40wo61WdeoA7NM/zaoq1/2cs13M7GyiF+T4rxuA4xM+6LeHWjWbss7hkGXjFDRcKD4O04Q==
dependencies:
"@typescript-eslint/types" "4.22.0"
"@typescript-eslint/visitor-keys" "4.22.0"

"@typescript-eslint/scope-manager@4.4.0":
version "4.4.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/scope-manager/-/scope-manager-4.4.0.tgz"
Expand All @@ -1802,11 +1822,29 @@
"@typescript-eslint/types" "4.4.0"
"@typescript-eslint/visitor-keys" "4.4.0"

"@typescript-eslint/types@4.22.0":
version "4.22.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/types/-/types-4.22.0.tgz#0ca6fde5b68daf6dba133f30959cc0688c8dd0b6"
integrity sha512-sW/BiXmmyMqDPO2kpOhSy2Py5w6KvRRsKZnV0c4+0nr4GIcedJwXAq+RHNK4lLVEZAJYFltnnk1tJSlbeS9lYA==

"@typescript-eslint/types@4.4.0":
version "4.4.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/types/-/types-4.4.0.tgz"
integrity sha512-nU0VUpzanFw3jjX+50OTQy6MehVvf8pkqFcURPAE06xFNFenMj1GPEI6IESvp7UOHAnq+n/brMirZdR+7rCrlA==

"@typescript-eslint/typescript-estree@4.22.0":
version "4.22.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/typescript-estree/-/typescript-estree-4.22.0.tgz#b5d95d6d366ff3b72f5168c75775a3e46250d05c"
integrity sha512-TkIFeu5JEeSs5ze/4NID+PIcVjgoU3cUQUIZnH3Sb1cEn1lBo7StSV5bwPuJQuoxKXlzAObjYTilOEKRuhR5yg==
dependencies:
"@typescript-eslint/types" "4.22.0"
"@typescript-eslint/visitor-keys" "4.22.0"
debug "^4.1.1"
globby "^11.0.1"
is-glob "^4.0.1"
semver "^7.3.2"
tsutils "^3.17.1"

"@typescript-eslint/typescript-estree@4.4.0":
version "4.4.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/typescript-estree/-/typescript-estree-4.4.0.tgz"
Expand All @@ -1821,6 +1859,14 @@
semver "^7.3.2"
tsutils "^3.17.1"

"@typescript-eslint/visitor-keys@4.22.0":
version "4.22.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/visitor-keys/-/visitor-keys-4.22.0.tgz#169dae26d3c122935da7528c839f42a8a42f6e47"
integrity sha512-nnMu4F+s4o0sll6cBSsTeVsT4cwxB7zECK3dFxzEjPBii9xLpq4yqqsy/FU5zMfan6G60DKZSCXAa3sHJZrcYw==
dependencies:
"@typescript-eslint/types" "4.22.0"
eslint-visitor-keys "^2.0.0"

"@typescript-eslint/visitor-keys@4.4.0":
version "4.4.0"
resolved "https://registry.yarnpkg.com/@typescript-eslint/visitor-keys/-/visitor-keys-4.4.0.tgz"
Expand Down Expand Up @@ -4240,6 +4286,13 @@ eslint-plugin-react@^7.21.3:
resolve "^1.17.0"
string.prototype.matchall "^4.0.2"

eslint-plugin-testing-library@^4.1.2:
version "4.1.2"
resolved "https://registry.yarnpkg.com/eslint-plugin-testing-library/-/eslint-plugin-testing-library-4.1.2.tgz#f986bea35059118f3f37fb750b230b6a0fb0aa52"
integrity sha512-eJR1drH2yFcGM9As3C5Q17lyOolMPKalQA3l8pD61sAIt8+8w/KjHe7u9Sa4eYy7r76UikCoP9CAtVkGdGCZ/w==
dependencies:
"@typescript-eslint/experimental-utils" "^4.21.0"

eslint-scope@^4.0.3:
version "4.0.3"
resolved "https://registry.yarnpkg.com/eslint-scope/-/eslint-scope-4.0.3.tgz"
Expand Down