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

feat: light UI redesign #30

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 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
7 changes: 4 additions & 3 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@ module.exports = {
'plugin:jest/recommended',
],
rules: {
'no-console': 'off',
'comma-dangle': 'off',
'implicit-arrow-linebreak': 'off',
'import/prefer-default-export': '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.

sortin things alphabetically

Copy link
Collaborator Author

@postrad postrad Jun 20, 2020

Choose a reason for hiding this comment

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

and we prefer named imports 😄

'jsx-a11y/label-has-associated-control': 'off',
'react/prop-types': 'off',
'react-hooks/rules-of-hooks': 'error', // Checks rules of Hooks
'no-console': 'off',
'react-hooks/exhaustive-deps': 'warn', // Checks effect dependencies
'react-hooks/rules-of-hooks': 'error', // Checks rules of Hooks
'react/prop-types': 'off',
},
};
4 changes: 2 additions & 2 deletions src/components/__tests__/customInput.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ describe('customInput', () => {
it('should update the current color with a custom one', () => {
// Learn more about onSubmit testing here: https://kula.blog/posts/test_on_submit_in_react_testing_library/
const saveSpy = jest.fn();
const { getByLabelText } = render(<CustomInput color="#ffffff" saveHandler={saveSpy} />);
const { getByLabelText, getByTestId } = render(<CustomInput color="#ffffff" saveHandler={saveSpy} />);

const input = getByLabelText('custom input') as HTMLInputElement;
const form = getByLabelText('custom form');
const form = getByTestId('form');

fireEvent.change(input, {
target: { value: '#f06d06' },
Expand Down
62 changes: 9 additions & 53 deletions src/components/__tests__/popup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import {
RenderResult,
waitFor,
fireEvent,
getAllByTestId,
} from '@testing-library/react';
import { act } from 'react-dom/test-utils';

import * as chromeHelpers from '../../helpers/chrome';
import Popup, { colors } from '../popup';
import { COLORS } from '../../constants/colors';
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 like the app caps constant.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed 👍

import Popup from '../popup';

jest.mock('../../helpers/chrome.ts');
jest.mock('../../helpers/whitelisting.ts');
Expand All @@ -25,63 +27,17 @@ describe('Popup', () => {
utils = render(<Popup />);
});

const { queryAllByLabelText } = utils;
const { queryAllByLabelText, getByTestId, getAllByTestId } = utils;

(chromeHelpers.getActiveColor as jest.Mock).mockResolvedValue(colors[0]);
(chromeHelpers.getActiveColor as jest.Mock).mockResolvedValue(COLORS[0]);

await waitFor(() => expect(chromeHelpers.getActiveColor).toHaveBeenCalled());

const colorButtons = queryAllByLabelText('save color');
const colorButtons = getAllByTestId('color button');
const whiteList = queryAllByLabelText('whitelist toggle');
const customForm = queryAllByLabelText('custom form');

expect(colorButtons[0]).toHaveClass('colorLinks--button active');
expect(whiteList.length).toEqual(1);
expect(customForm.length).toEqual(0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Custom form exists in the DOM now, even when it isn't visible.

});

it('should show custom input when no active color found', async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Active color always exists, so I removed this test.

let utils = {} as RenderResult;

act(async () => {
utils = render(<Popup />);
});

const { getByLabelText, queryAllByLabelText, container } = utils;

(chromeHelpers.getActiveColor as jest.Mock).mockResolvedValue('');

await waitFor(() => expect(chromeHelpers.getActiveColor).toHaveBeenCalled());

const customColor = getByLabelText('custom color');
const customForm = queryAllByLabelText('custom form');

expect(customColor).toHaveClass('colorLinks--button active');
await waitFor(() => expect(customForm.length).toEqual(1), { container });
});

it('should show a custom input when the custom color button is clicked', async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This functionality no longer exists.

let utils = {} as RenderResult;

act(async () => {
utils = render(<Popup />);
});

const { queryAllByLabelText, queryByLabelText } = utils;
(chromeHelpers.getActiveColor as jest.Mock).mockResolvedValue(colors[0]);

await waitFor(() => expect(chromeHelpers.getActiveColor).toHaveBeenCalled());

const colorButtons = queryAllByLabelText('save color');
const customColorButton = queryByLabelText('custom color');
const customForm = queryAllByLabelText('custom form');

expect(colorButtons[0]).toHaveClass('colorLinks--button active');
expect(customForm.length).toEqual(0);

fireEvent.click(customColorButton);

waitFor(() => expect(customForm.length).toEqual(1));
});

it('should save a color when a color button is clicked', async () => {
Expand All @@ -91,12 +47,12 @@ describe('Popup', () => {
utils = render(<Popup />);
});

const { queryAllByLabelText } = utils;
(chromeHelpers.getActiveColor as jest.Mock).mockResolvedValue(colors[0]);
const { queryAllByLabelText, getAllByTestId } = utils;
(chromeHelpers.getActiveColor as jest.Mock).mockResolvedValue(COLORS[0]);

await waitFor(() => expect(chromeHelpers.getActiveColor).toHaveBeenCalled());

const colorButtons = queryAllByLabelText('save color');
const colorButtons = getAllByTestId('color button');
const customForm = queryAllByLabelText('custom form');

expect(colorButtons[0]).toHaveClass('colorLinks--button active');
Expand Down
4 changes: 2 additions & 2 deletions src/components/colorButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ interface OwnProps {
const ColorButton: FC<OwnProps> = ({ color, clickHandler, active }) => (
<div>
<button
aria-label="save color"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need an aria-label, since this is clearly an extension that assists people who can see, and this label would only benefit screen reader users.

data-testid="color button"
type="button"
className={`colorLinks--button${active ? ' active' : ''}`}
onClick={clickHandler}
style={{ color: 'white', background: color }}
style={{ background: color }}
/>
</div>
);
Expand Down
25 changes: 16 additions & 9 deletions src/components/customInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,24 @@ const CustomInput: FC<OwnProps> = ({ color, saveHandler }) => {

return (
<form
aria-label="custom form"
className="colorLinks--custom"
className="customForm"
onSubmit={() => saveHandler(currentColor)}
data-testid="form"
>
<input
placeholder="#ff0000"
aria-label="custom input"
value={currentColor}
onChange={(e) => setColor(e.target.value)}
/>
<button type="submit">Save</button>
<label className="customForm--label">enter hexadecimal value</label>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: get the for and id hooked up for the input.

<div className="customForm--clickables">
<input
aria-label="custom input"
value={currentColor}
onChange={(e) => setColor(e.target.value)}
/>
{' '}
<button type="submit">save</button>
<p>
current color:
<b style={{ color }}>{color}</b>
</p>
</div>
</form>
);
};
Expand Down
37 changes: 13 additions & 24 deletions src/components/popup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,14 @@ import WhitelistManager from './whitelist';
import ColorButton from './colorButton';
import CustomInput from './customInput';
import { getActiveColor, saveActiveColor } from '../helpers/chrome';

export const colors = ['#37d67a', '#2ccce4', '#06A77D', '#ff8a65', '#1E91D6'];
import { COLORS } from '../constants/colors';

const Popup = () => {
const [activeColor, setActiveColor] = useState('');
const [showCustomInput, setShowCustomInput] = useState(false);

const onColorChange = useCallback((color) => {
saveActiveColor(color);
setActiveColor(color);
setShowCustomInput(false);
}, []);

useEffect(() => {
Expand All @@ -28,46 +25,38 @@ const Popup = () => {
}

setActiveColor(result);
setShowCustomInput(!colors.includes(result));
}

getActiveColorEffect();
}, []);

return (
<div className="colorLinks">
<span
<h1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Preserve the nice visual indicator for the current color, but also let's remind users what extension they're using.

style={{
color: activeColor,
paddingBottom: '5px',
transition: 'color .5s',
}}
}}
>
A Quick Brown Fox Jumped
</span>
<img aria-hidden="true" src="../links48.png" alt="" />
<span>color links</span>
</h1>
<div className="colorLinks--grid">
{colors.map((color) => (
{COLORS.map((color) => (
<ColorButton
color={color}
clickHandler={() => onColorChange(color)}
key={color}
active={color === activeColor}
/>
))}

<div>
<button
type="button"
aria-label="custom color"
className={`colorLinks--button${!colors.includes(activeColor) ? ' active' : ''}`}
onClick={() => setShowCustomInput(true)}
>
#
</button>
</div>
</div>

{showCustomInput && <CustomInput color={activeColor} saveHandler={onColorChange} />}
<details>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still need to refactor (remove?) the hook logic, since the sweet new <details> element makes this easy.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh that is 🔥

<summary>
custom color
</summary>
<CustomInput color={activeColor} saveHandler={onColorChange} />
</details>
<WhitelistManager />
</div>
);
Expand Down
2 changes: 2 additions & 0 deletions src/constants/colors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

export const COLORS: Array<string> = ['#37d67a', '#2ccce4', '#06A77D', '#ff8a65', '#1E91D6'];
Loading