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

CSS modifications #60

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Rukmini-Meda
Copy link

Fixes #4

Changes done in this pull request:

Changed styling of the input fields

Files Added:

No new files added or removed.
index.js and App.css files changed

Screenshots/Links:

Screenshot (139)

Screenshot (140)

Screenshot (141)

System Information :

  • OS: Windows
  • Browser: Google Chrome
  • Version: 81.0.4044.129 (Official Build) (64-bit)

Reviewers:

@AdityaSrivast Please review my PR. I made style changes for input fields and error dialog. Please let me know if I have to make any more further changes or improvements.

We appreciate you help towards improving the module, we look forward to having more contributions from your end

@Rukmini-Meda
Copy link
Author

@AdityaSrivast Please review my PR. Please let me know if I have to improve anything.

Copy link
Member

@AdityaSrivast AdityaSrivast left a comment

Choose a reason for hiding this comment

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

Also get rid of package-lock.json changes.

@@ -8,6 +8,7 @@
"@testing-library/user-event": "^7.1.2",
"react": "^16.13.0",
"react-dom": "^16.13.0",
"react-pincode": "^1.0.2",
Copy link
Member

@AdityaSrivast AdityaSrivast May 3, 2020

Choose a reason for hiding this comment

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

@Rukmini-Meda there's no need to install the module in the test directory. Get rid of this line.

@@ -10,10 +15,43 @@ input::-webkit-inner-spin-button {

/* Firefox */
input[type=number] {
-moz-appearance: textfield;
/* -moz-appearance: textfield; */
Copy link
Member

Choose a reason for hiding this comment

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

@Rukmini-Meda any reason why this line is commented?

Copy link
Member

Choose a reason for hiding this comment

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

@Rukmini-Meda
Copy link
Author

@AdityaSrivast I made changes as you said. I removed the package-lock.json file, removed the line you said in package.json. And about the line that's commented, when I started to implement my styling I kinda commented it to check what I was doing. Its completely fine even if its not commented. Please review for any more changes and merge. Thank you!

@Rukmini-Meda
Copy link
Author

@AdityaSrivast Can you merge this PR if there are no other changes or improvements required?

@AdityaSrivast
Copy link
Member

@Rukmini-Meda get rid of package-lock.json. Also if possible give any codesandbox, or jsfiddle link so that your implementation can be verified

@Rukmini-Meda
Copy link
Author

@AdityaSrivast Sure, I will update it by tomorrow.

@Rukmini-Meda
Copy link
Author

@AdityaSrivast Can you help me with codesandbox? I imported my branch into it. But build/index.js shows many problems although it works fine in vscode. This is the link for codesandbox - https://codesandbox.io/s/distracted-yonath-6l27d?file=/build/index.js:0-26767

@Rukmini-Meda
Copy link
Author

@AdityaSrivast Can you help me with codesandbox? I imported my branch into it. But build/index.js shows many problems although it works fine in vscode. This is the link for codesandbox - https://codesandbox.io/s/distracted-yonath-6l27d?file=/build/index.js:0-26767

@AdityaSrivast Can you help with this so that the PR can be merged as we have only 2 days left?

Copy link
Collaborator

@yellowwoods12 yellowwoods12 left a comment

Choose a reason for hiding this comment

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

@Rukmini-Meda If it is working fine in vs-code then you must've done some mistake in implementing it in Code Sandbox. I reviewed it and was able to solve around 50 errors just by formatting it properly. Try implementing it again using some proper guide.

@Rukmini-Meda
Copy link
Author

@yellowwoods12 Can you suggest any resources?

@Rukmini-Meda
Copy link
Author

@yellowwoods12 @AdityaSrivast Can you please help me with codesandbox so that I can complete this PR successfully?

@AdityaSrivast
Copy link
Member

AdityaSrivast commented May 31, 2020

@Rukmini-Meda use test-server github url while importing, and add the code for module like Pincode.js and paste your code from App.js to it (do not install react-pincode as dependency, otherwise your changes will not appear). Also add your css file. That should do the trick.

Resolve conflicts and make the changes which I asked previously and we'll merge it through. 👍

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.

Improve CSS of all the input fields.
3 participants