feat: change password view #1932 #119
Conversation
…n_js into feat--change-password-view-#1932
|
✨ Chromatic and Storybook are ready! 🎉 👉 Chromatic Link triggered by a8c15f9 af509cd3-e034-490e-9a0e-93625c124f76 |
|
✅ Vercel preview is ready! 🎉 triggered by a8c15f9 e283ce82-0cf8-4055-bc5c-8e93da828c8e |
| @@ -81,3 +81,12 @@ export const Header = styled.h1` | |||
| margin-bottom: none; | |||
| } | |||
| `; | |||
|
|
|||
| export const ChangePass = styled.span` | |||
There was a problem hiding this comment.
You should use an anchor or a button instead of imitating a link with a span, this is a bad practice.
Since it doesn't navigate away, it should be a button.
There was a problem hiding this comment.
Okay, since we don't have a button with text only, should I use our Button component or normal button tag? Of course both types would be properly styled.
There was a problem hiding this comment.
Our Button, onlyIcon prop gets you half way there, the prop should be renamed to something more fitting its actual role which is transparent really (no background, no border). Adding a prop such as asLink would be confusing since the button acts as a Link if href is passed but you don't want href.
Try creating a component that extends Button, sets attrs { onlyIcon: true} and adds the style for the text. See the pencil icon for how to do it.
| const Wrapper = styled.div` | ||
| width: 100%; | ||
| padding: 1em 0; | ||
| `; | ||
|
|
||
| export { Wrapper }; |
There was a problem hiding this comment.
| const Wrapper = styled.div` | |
| width: 100%; | |
| padding: 1em 0; | |
| `; | |
| export { Wrapper }; | |
| export const Wrapper = styled.div` | |
| width: 100%; | |
| padding: 1em 0; | |
| `; |
| return <ChangePassword {...args} />; | ||
| }; | ||
|
|
||
| export const Responsive = Template.bind(); |
There was a problem hiding this comment.
| export const Responsive = Template.bind(); | |
| export const Responsive = Template.bind({}); |
| const initialState = Object.freeze({ currentPass: '', newPass: '', repeatPass: '' }); | ||
|
|
||
| export default function ChangePassword() { | ||
| const [inputValues, setInputValues] = useState(initialState); |
There was a problem hiding this comment.
Add validation and prepare payload for later.
| @@ -0,0 +1,41 @@ | |||
| import { Input } from 'components/Form'; | |||
| import { Wrapper } from './ChangePassword.styles'; | |||
| import { useState } from 'react'; | |||
There was a problem hiding this comment.
This useState import should be the first on the list.
There was a problem hiding this comment.
After review changes I don't use useState in that file :)
There was a problem hiding this comment.
Right, but the same situation is in ChangePassword.stories.js file.:)
You mean to make underline visible all the time, not only on hover? |
Maybe underlining is not the best, hmm but for example higher font-size and font-weight value? But it's only suggestion so you can try it and decide. ;) |
| @@ -31,6 +31,7 @@ export const FlexRow = styled.div` | |||
| flex-direction: row; | |||
| justify-content: space-evenly; | |||
| align-items: center; | |||
| min-width: 375px; | |||
There was a problem hiding this comment.
Same issue here:
There was a problem hiding this comment.
On a specific device or on all of them?
| cursor: pointer; | ||
| font-weight: bold; | ||
| &:hover { | ||
| text-decoration: underline; | ||
| } | ||
| margin: 12px; | ||
| color: #44ec52; | ||
| `; |
There was a problem hiding this comment.
Perhaps we could unify these buttons:
No description provided.