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

UI improvement on Home Page #40

Closed

Conversation

shivambalwani
Copy link

issue #3 :
Added a typing effect on "Exploring possibilities" .
@pranshuchittora is it good or i need to add some changes to it?

@netlify
Copy link

netlify bot commented Mar 12, 2021

Deploy preview for rne-playground processing.

Building with commit fe005f8

https://app.netlify.com/sites/rne-playground/deploys/6055a5b5c5274e00071023c5

</Typography>
<Typography
variant="h6"
style={{ fontWeight: "200", fontStyle: "italic" }}
className={styles.headerbottom__home}
>
with React Native Elements
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to make it a link which redirects to RNE main repo.

Copy link
Contributor

@Uyadav207 Uyadav207 left a comment

Choose a reason for hiding this comment

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

@shivambalwani,

Problems

  • Direct CSS used.
  • Package-lock.json conflict

Solution

  • try making CSS modules
  • Delete package-lock.json and commit changes on your PR branch

@Uyadav207
Copy link
Contributor

@shivambalwani , Great

@shivambalwani
Copy link
Author

shivambalwani commented Mar 12, 2021

@shivambalwani,

Problems

  • Direct CSS used.
  • Package-lock.json conflict

Solution

  • try making CSS modules
  • Delete package-lock.json and commit changes on your PR branch

I have added CSS modules & removed package-lock.json. Thanks!

Copy link
Contributor

@tewarig tewarig left a comment

Choose a reason for hiding this comment

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

well, we can also directly add the typewriting effect. instead of using another package. using {useEfect, useState} hooks.
check out this code snippet.
https://gist.githubusercontent.com/abecus/7d20f05a140e99088d9f37517bd594c3/raw/28e0a625691ee655d097fb68cff09f1dd9168ae1/typewriter.js

@shivambalwani
Copy link
Author

well, we can also directly add the typewriting effect. instead of using another package. using {useEfect, useState} hooks.
check out this code snippet.
https://gist.githubusercontent.com/abecus/7d20f05a140e99088d9f37517bd594c3/raw/28e0a625691ee655d097fb68cff09f1dd9168ae1/typewriter.js

yes, we can but using this package provides more effects which can be applied quite easily when the user needs.

@pranshuchittora
Copy link
Member

Typing effect is not required & styles are breaking on the home page (spacing issue)

@shivambalwani
Copy link
Author

Typing effect is not required & styles are breaking on the home page (spacing issue)

Ok, I will remove the typing effect .But i am not able to understand where the styles are breaking 🤔

@pranshuchittora
Copy link
Member

https://deploy-preview-40--rne-playground.netlify.app/#/
Check the section below Explore Now button, compare the same with this -> https://deploy-preview-40--rne-playground.netlify.app/#/

@shivambalwani
Copy link
Author

@pranshuchittora fixed the style breaking issue and removed the typing effect.plz check.

package.json Outdated
@@ -22,6 +22,7 @@
"react-native-web-linear-gradient": "^1.1.1",
"react-router-dom": "^5.2.0",
"react-scripts": "3.4.1",
"react-typewriter": "^0.4.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

you did remove the typing effect but forgot to uninstall this package.
npm uninstall react-typewriter

border: none;
border-top: 1px solid rgb(226, 226, 226);
}
.image{
Copy link
Member

Choose a reason for hiding this comment

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

This is too generic name.

margin-top: 1rem;
}

.line{
Copy link
Member

Choose a reason for hiding this comment

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

This is too generic name.

}


.paper{
Copy link
Member

Choose a reason for hiding this comment

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

This is too generic name.

text-align: center;
margin-bottom: 2rem;
}
.divider{
Copy link
Member

Choose a reason for hiding this comment

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

This is a too generic name.

Copy link
Member

@pranshuchittora pranshuchittora left a comment

Choose a reason for hiding this comment

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

Left some comments.

@shivambalwani
Copy link
Author

Left some comments.

@pranshuchittora i have fixed the style naming, but the checks are failing. I don't know why?

@pranshuchittora
Copy link
Member

Pls pull the new changes and resolve the conflicts.

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.

6 participants