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

1664 ui make homepage mobile friendly #1692

Merged

Conversation

naima-shk
Copy link
Contributor

Bugzilla Bug: XXXXX
@helfi92

@naima-shk naima-shk requested a review from a team as a code owner October 18, 2019 22:02
@@ -20,6 +20,9 @@ import SignInDialog from '../SignInDialog';
[theme.breakpoints.down('sm')]: {
marginBottom: theme.spacing.double,
},
'@media (max-width: 400px)': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use material-ui breakpoints instead of hard-coding width values. See line above for an example.

@@ -34,6 +37,11 @@ import SignInDialog from '../SignInDialog';
fill: theme.palette.common.white,
marginRight: theme.spacing.unit,
},
text: {
'@media (max-width: 400px)': {
Copy link
Contributor

Choose a reason for hiding this comment

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

See other comment.

@@ -20,6 +20,9 @@ import SignInDialog from '../SignInDialog';
[theme.breakpoints.down('sm')]: {
marginBottom: theme.spacing.double,
},
'@media (max-width: 400px)': {
fontSize: '40px',
Copy link
Contributor

Choose a reason for hiding this comment

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

fontSize: 40,

@@ -34,6 +37,11 @@ import SignInDialog from '../SignInDialog';
fill: theme.palette.common.white,
marginRight: theme.spacing.unit,
},
text: {
'@media (max-width: 400px)': {
fontSize: '20px',
Copy link
Contributor

Choose a reason for hiding this comment

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

fontSize: 20,

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Tests are failing. You can fix the linting error with yarn lint --fix inside the ui directory. Also, you'll need to add a changelog with yarn changelog --silent --issue 1664 from the root directory (not the ui directory).

@naima-shk naima-shk requested a review from a team as a code owner October 22, 2019 14:02
Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Looks really good. Just minor changes then we also need to rebase on master and fix the linting issues inside the ui directory.

@@ -34,6 +35,11 @@ import SignInDialog from '../SignInDialog';
fill: theme.palette.common.white,
marginRight: theme.spacing.unit,
},
text: {
[theme.breakpoints.down('sm')]: {
fontSize: 20
Copy link
Contributor

Choose a reason for hiding this comment

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

fontSize: 16,

@@ -34,6 +35,11 @@ import SignInDialog from '../SignInDialog';
fill: theme.palette.common.white,
marginRight: theme.spacing.unit,
},
text: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename class to ciDescription.

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

@naima-shk
Copy link
Contributor Author

@helfi92 i made the changes you requested ! :)

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

🎉

@helfi92 helfi92 merged commit 5d28247 into taskcluster:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants