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

Feature/redesign #37

Merged
merged 18 commits into from
Oct 21, 2019
Merged

Feature/redesign #37

merged 18 commits into from
Oct 21, 2019

Conversation

CarlosMed
Copy link
Contributor

@CarlosMed CarlosMed commented Oct 19, 2019

Created a redesign of a design I quickly sketched up. Attached is a quick link to the app in its current state. It's responsive for both mobile consumption to desktop. This resolves the styling portion for issue #33 but not really the animation.

@Arcaster42
Copy link
Owner

Thanks for your work on this! Can you take a look at the conflict and work it in with the latest changes?

@CarlosMed
Copy link
Contributor Author

I sure can.

@CarlosMed
Copy link
Contributor Author

@Arcaster42 Ok, I think I'm done with redesign. One thing I would say is that I do have a redesign for a dark theme as well that can be implemented. There's is also a better design for incorrect guesses but seeing as the app has gone away from the 5 strikes you're out method and more towards a how many can you complete in 30sec what is currently there should suffice with the current design. Let me know if you have any questions.

Copy link
Owner

@Arcaster42 Arcaster42 left a comment

Choose a reason for hiding this comment

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

Thanks for all your professional effort in this!

Comment on lines +2 to +15
:root {
--bg: #dfdfdf;
--primary: #3182a6;
--danger: #ef5350;
--white: #fff;
--light-grey: #B0B0B0;
--hint-grey: #EFEFEF;
--primary-gradient: linear-gradient(166.21deg, #2a8c92 2.56%, #347bb2 122.3%);
--s-border-radius: 3px;
--l-border-radius: 8px;
--base-font: 16px;
--font: 'Roboto', sans-serif;
--drop-shadow: 0px 4px 12px rgba(42, 101, 146, 0.4);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Excellent addition showing how root can be used to declare global variables in CSS.

Copy link
Contributor Author

@CarlosMed CarlosMed Oct 21, 2019

Choose a reason for hiding this comment

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

I had to do with what I got and CSS variables were more viable than introducing a whole build system just for sass. Plus it allows easy theme edit like a dark theme and easier migration to any other pre-processor

Comment on lines -92 to -109
@keyframes shake {
10%,
90% {
transform: translate3d(-1px, -2px, 0);
}
20%,
80% {
transform: translate3d(2px, 3px, 0);
}
30%,
50%,
70% {
transform: translate3d(-4px, -3px, 0);
}
40%,
60% {
transform: translate3d(4px, 3px, 0);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I loved shake, but sometimes it's best to defer these kind of things to other developers.

Copy link
Contributor Author

@CarlosMed CarlosMed Oct 21, 2019

Choose a reason for hiding this comment

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

Shake can be used at a smaller scale since its such a big and rapid movement from a UX standpoint it's not the best. Shake can be put on the input for example since its small and minimal but I wouldn't put shake for correct answers as shake is more imposed for something wrong or to bring attention to something. So I just removed it for correct answers and left them on the card.

Comment on lines -35 to 37
const startButton = document.getElementById('start')
const restartButton = document.getElementById('restart')
const startButton = document.querySelector('.header_button_start')
const restartButton = document.querySelector('.header_button_restart')

Copy link
Owner

Choose a reason for hiding this comment

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

For anyone curious about getElementById vs querySelector, check out this post on StackOverflow!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I choose to go with querySelector due to the fact that performance isn't an issue for something like this. Now if it gets monolithic there can potentially be an issue but I have worked in bigger scaled applications and that doesn't seem to cause bottlenecks if imposed correctly but its all personal preference. Also with querySelector or querySelectorAll, you can target things like you would in CSS so for example document.querySelector(".button[disabled=true]").

tldr: In the end its personal preference depending on how cognitive you are on performance and making sure it functions across browser by crosschecking in caniuse.

@Arcaster42 Arcaster42 merged commit e8c4a2b into Arcaster42:master Oct 21, 2019
@CarlosMed
Copy link
Contributor Author

Glad I could help. Feel free to message me if you need help with anything else. 😄

@CarlosMed CarlosMed deleted the feature/redesign branch October 21, 2019 03:49
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.

2 participants