-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: master
Are you sure you want to change the base?
Conversation
@@ -8,7 +8,8 @@ import { | |||
import { act } from 'react-dom/test-utils'; | |||
|
|||
import * as chromeHelpers from '../../helpers/chrome'; | |||
import Popup, { colors } from '../popup'; | |||
import { COLORS } from '../../constants/colors'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 👍
src/components/customInput.tsx
Outdated
onChange={(e) => setColor(e.target.value)} | ||
/> | ||
<button type="submit">Save</button> | ||
<label className="customForm--label">enter hexadecimal value</label> |
There was a problem hiding this comment.
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.
} | ||
|
||
getActiveColorEffect(); | ||
}, []); | ||
|
||
return ( | ||
<div className="colorLinks"> | ||
<span | ||
<h1 |
There was a problem hiding this comment.
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.
</div> | ||
|
||
{showCustomInput && <CustomInput color={activeColor} saveHandler={onColorChange} />} | ||
<details> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that is 🔥
|
||
/* variables | ||
_________________________________________________________*/ | ||
:root { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to use css custom properties for all the color logic. Setting and getting them in a Chrome extension has proven a little more elusive than I expected. But this will at least help consolidate our style values.
@@ -8,7 +8,8 @@ import { | |||
import { act } from 'react-dom/test-utils'; | |||
|
|||
import * as chromeHelpers from '../../helpers/chrome'; | |||
import Popup, { colors } from '../popup'; | |||
import { COLORS } from '../../constants/colors'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 👍
</div> | ||
|
||
{showCustomInput && <CustomInput color={activeColor} saveHandler={onColorChange} />} | ||
<details> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that is 🔥
src/constants/colors.ts
Outdated
@@ -0,0 +1,2 @@ | |||
|
|||
export const COLORS: Array<string> = ['#37d67a', '#2ccce4', '#06A77D', '#ff8a65', '#1E91D6'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can turn prefer default export
out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know, I don't mind named imports. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer them, to be honest. LETS BREAK THEM DOWN!
expect(customForm.length).toEqual(0); | ||
}); | ||
|
||
it('should show custom input when no active color found', async () => { |
There was a problem hiding this comment.
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.
|
||
expect(colorButtons[0]).toHaveClass('colorLinks--button active'); | ||
expect(whiteList.length).toEqual(1); | ||
expect(customForm.length).toEqual(0); |
There was a problem hiding this comment.
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.
await waitFor(() => expect(customForm.length).toEqual(1), { container }); | ||
}); | ||
|
||
it('should show a custom input when the custom color button is clicked', async () => { |
There was a problem hiding this comment.
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.
@@ -9,7 +9,7 @@ interface OwnProps { | |||
const ColorButton: FC<OwnProps> = ({ color, clickHandler, active }) => ( | |||
<div> | |||
<button | |||
aria-label="save color" |
There was a problem hiding this comment.
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.
'comma-dangle': 'off', | ||
'implicit-arrow-linebreak': 'off', | ||
'import/prefer-default-export': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sortin things alphabetically
There was a problem hiding this comment.
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 😄
Pull Request Test Coverage Report for Build 143157397
💛 - Coveralls |
@@ -16,6 +16,7 @@ module.exports = { | |||
'comma-dangle': 'off', | |||
'implicit-arrow-linebreak': 'off', | |||
'import/prefer-default-export': 'off', | |||
'jsx-a11y/control-has-associated-label': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule seems unnecessary to enforce since screen reader support doesn't seem necessary.
Wow @postrad, these are some really awesome upgrades! I love the new UI tweaks too. I did see one "bug" (if you will), where submitting a custom color would not "save", and wouldn't display "current color". Also, do you think it'd be helpful to support both |
The current UI isn't quite as intuitive as it could be. This PR aims to help fix that problem.
make "white list" feature's behavior clearerI think, after spending time with the UI during this PR, that this is out of scope. It deserves a PR of its own.
This is currently a work-in-progress. I still need to review open issues to see if this PR can address those any of those as well.