Changed:#14
Conversation
zaverden
left a comment
zaverden
left a comment
There was a problem hiding this comment.
Thank you for your submission.
There are 2 main reasons of my comments
- Outdated design document. Unfortunately I did not ask designer to update the document according the latest view
- changes in another modules. I don't want to have extra changes that are not related to the fixing issue. you can save these changes for another PR.
and before making changes by my comments please rebase onto tag 1.4.0 - there are som changes you don't have in your branch.
| border: none; | ||
| border-radius: 4px; | ||
| font-weight: var(--fw-b); | ||
| font-weight: var(--fw-th); |
There was a problem hiding this comment.
this does not match design. buttons has 700 by default
| transition: 0.3s; | ||
|
|
||
| &:hover { | ||
| opacity: 0.5; | ||
| } |
There was a problem hiding this comment.
it's nice to have this extra interactivity on hover. but I don't like to achieve it by opacity.
I'd rather go with box-shadow or/and separate color for hover. revert for now
| const Logo = styled.img` | ||
| display: block; | ||
| margin: 0 auto; | ||
| max-width: 60%; | ||
| max-width: 210px; | ||
| width: 100%; | ||
| height: auto; | ||
| `; | ||
|
|
||
| const Slogan = styled.p` | ||
| display: block; | ||
| margin: 0 auto; | ||
| margin: 15px auto 40px; | ||
| width: fit-content; | ||
| font-size: var(--fs-s); | ||
| line-height: 2em; | ||
|
|
||
| font-weight: var(--fw-th); | ||
| line-height: 1.2; | ||
|
|
||
| & > span { | ||
| color: var(--fg-a); | ||
| font-weight: var(--fw-b); | ||
| } |
There was a problem hiding this comment.
I don't see why there are any changes in this file. please revert them
|
|
||
| & > h1 { | ||
| margin: 0 auto 25px; | ||
| font-size: 24px; | ||
| font-weight: bold; | ||
| } | ||
|
|
||
| & > p { | ||
| margin-top: 10px; | ||
| font-weight: var(--fw-th); | ||
| } |
There was a problem hiding this comment.
these changes does not relate to the issue nor they match design. please revert
| width: 22px; | ||
| height: 22px; | ||
| margin-right: 5px; |
There was a problem hiding this comment.
these changes does not relate to the issue nor they match design. please revert
| return ( | ||
| <div | ||
| className="ql-editor" | ||
| style={{ padding: "0", fontWeight: "500", fontSize: "18px", color: "#202020" }} |
There was a problem hiding this comment.
use styled for styles override. and let's remove side paddings only. the rest look fine already
| font-size: var(--fs-xs); | ||
| color: var(--fg-m); | ||
| `; | ||
|
|
||
| const Field = styled.p` | ||
| font-size: var(--fs-xl); | ||
| color: var(--fg-p); | ||
| margin-top: 0; | ||
| margin-bottom: 16px; |
| disabled={isSaving} | ||
| style={{ height: "44px", marginTop: "18px", background: "#515151" }}> | ||
| Create event | ||
| </Button> |
There was a problem hiding this comment.
I expect to see these changes for #2. please save these changes for another PR
| {join.isError ? <p>{join.error?.message}</p> : null} | ||
| {join.isSuccess ? ( | ||
| <p>You have successfully joined</p> | ||
| <JoinedSuccessfully>You have successfully joined</JoinedSuccessfully> |
There was a problem hiding this comment.
I'm sure the changes in this file don't belong to this PR. please revert them
| font-weight: normal; | ||
| color: #00A52E; | ||
| background: #E4FFE3; | ||
| `; |
There was a problem hiding this comment.
this panel is out of app styles.
proper success message is in confirm-email-page
*View list event; *Form event; bugFix zaverden#1
bugFix zaverden#2
| font-size: var(--fs-s); | ||
| line-height: 2em; | ||
|
|
||
| --bg-p: #00AA98; | ||
| --bg-s: #515151; | ||
| --bg-m: #FFFFFF; | ||
| --bg-l: #00B5A1; |
There was a problem hiding this comment.
these new colors are not used. remove them
| </button> | ||
| </form> | ||
| <form onSubmit={onFormSubmit}> | ||
| <h2>Create New Event</h2> |
| <Button type="submit" | ||
| disabled={isSaving} | ||
| style={{ height: "44px", marginTop: "18px", background: "var(--bg-s)" }}> | ||
| Create event |
There was a problem hiding this comment.
user see this button when creating and editing. It's confusing to see "Create event" when I'm editing. Rename it to "Save event"
| </div> | ||
| <Button type="submit" | ||
| disabled={isSaving} | ||
| style={{ height: "44px", marginTop: "18px", background: "var(--bg-s)" }}> |
There was a problem hiding this comment.
do not set color in style. there is secondary prop to set this color.
and remove height - it is ok how it looks by default from ui-kit
|
|
||
| export const Input = styled.input` | ||
| width: 100%; | ||
| height: 40px; |
There was a problem hiding this comment.
I don't like the idea of fixing a height. remove this, it looks good enough
| box-sizing: border-box; | ||
| padding: 1em; | ||
| font-size: var(--fs-m); | ||
| background: transparent; |
|
|
||
| export const Anchor = styled("a")` | ||
| color: var(--fg-a); | ||
| font-weight: var(--fw-b); |
There was a problem hiding this comment.
it should not be bold by default. it is bold only in events list - set it there
| export const Anchor = styled("a")` | ||
| color: var(--fg-a); | ||
| font-weight: var(--fw-b); | ||
| font-size: var(--fs-m); |
There was a problem hiding this comment.
font size should inherit from context
| font-size: var(--fs-m); | ||
| text-decoration: none; | ||
| display: inline-block; | ||
| padding: 0 0 11px; |
There was a problem hiding this comment.
display and padding don't make sense here for me. give a reason for these changes or revert them
3 participants
*View list event;
*Form event;
bugFix #1