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

Migrate wallet header to final ui - Closes #1838 #1852

Merged
merged 21 commits into from Mar 26, 2019

Conversation

Projects
None yet
4 participants
@massao
Copy link
Contributor

massao commented Mar 25, 2019

What issue have I solved?

#1838

How have I implemented/fixed it?

Migrate wallet header to final UI.

  • Added new basier-circle font-family, set the font-weight to the same on sketch to use correct font;
  • Added font mixins for having consistent font across the application;
  • Created ByteCounter component to show a rounded progress bar for input fields;
  • Added new css variables based on the styleguide;
  • Updated buttons to new ones.

How has this been tested?

Go to /wallet and the header should be with the final UI, fonts and colors should be the final UI, also tabs should be with the final UI.

Review checklist

massao added some commits Mar 25, 2019

@massao massao self-assigned this Mar 25, 2019

massao added some commits Mar 25, 2019

@massao massao requested review from osvaldovega and michaeltomasik Mar 25, 2019

@massao massao changed the title 1838 migrate wallet header to final ui Migrate wallet header to final ui - Closes #1838 Mar 25, 2019

massao added some commits Mar 25, 2019

@osvaldovega
Copy link
Contributor

osvaldovega left a comment

all good, just a few minor changes.
Well done

@@ -1,5 +1,5 @@
body {
background-color: #013165;
background: #edf0f5;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

can you please use constants

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

The styles.head.css file wasn't using variables, so I kept like it was, using absolute values, and checking the webpack.config I think it isn't processed through postcss.

@@ -10,7 +10,7 @@ body {
top: 0;
z-index: -1;
overflow: hidden;
background-image: linear-gradient(-243deg, #f8fafd 0%, #eef2f7 95%);
background: #edf0f5;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

same here

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

same as above


@define-mixin contentSmallest {
font-family: var(--content-font);
font-size: 11px;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

same here

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

The idea of the mixins is to have the font stylized the same way in any place of the app, and these values shouldn't change based on the styleguide, so you should just use the mixin that fits your needs.


@define-mixin contentSmall $weight: normal {
font-family: var(--content-font);
font-size: 12px;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

same here

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

same as above


@define-mixin contentNormal $weight: normal {
font-family: var(--content-font);
font-size: 13px;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

same here

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

same as above


@define-mixin contentLarge $weight: normal {
font-family: var(--content-font);
font-size: 14px;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

here

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

same as above


@define-mixin contentLargest {
font-family: var(--content-font);
font-size: 15px;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

same here

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

same as above


&:focus,
&:hover {
background: #004aff !important;
background: color(var(--color-ultramarine-blue) + l(+4%)) !important;
box-shadow: 3px 3px 10px 0 rgba(61, 113, 255, 0.36) !important;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

this can be a constant

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

Not necessary right now, this box-shadow is being used only on the primary button component, and we shouldn't create lots of variables, because it's harder to maintain in the future.

&:disabled {
opacity: 0.8;
}

&:focus,
&:hover {
background: #ff6300 !important;

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Mar 26, 2019

Contributor

this too

This comment has been minimized.

Copy link
@massao

massao Mar 26, 2019

Author Contributor

We don't have the style for the danger button yet, thats why it's using the same old version

@osvaldovega
Copy link
Contributor

osvaldovega left a comment

ok I understand the reason.

@massao massao requested a review from Efefefef Mar 26, 2019

@Efefefef Efefefef requested a review from slaweet Mar 26, 2019

@Efefefef

This comment has been minimized.

Copy link
Contributor

Efefefef commented Mar 26, 2019

@slaweet Please do a review instead of me. For me, It looks different, not like in designs. I don't know what and where the final ones

@massao massao added the ready label Mar 26, 2019

@massao massao merged commit 5e4d00d into 1.15.0 Mar 26, 2019

5 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
coverage/coveralls Coverage remained the same at 94.069%
Details
security/snyk - package.json (LiskHQ) No manifest changes detected
@slaweet

This comment has been minimized.

Copy link
Member

slaweet commented Mar 26, 2019

@Efefefef I checked the PR, found a list of inconsistencies and talked to @massao to fix it in another PR as this one was already merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.