feat:Adding prettier as a dev dependency and formatting the whole code by abhishekkumar08 · Pull Request #59 · react-native-elements/playground

abhishekkumar08 · 2021-03-13T15:26:23Z

Fixes:#58

Fixes:react-native-elements#58

netlify · 2021-03-13T15:26:29Z

Deploy preview for rne-playground ready!

Built with commit 724040f

https://deploy-preview-59--rne-playground.netlify.app

Uyadav207 · 2021-03-13T15:31:46Z

.github/ISSUE_TEMPLATE/bug_report.md

 Example:
- 
+
 **Snack** : https://snack.expo.io/xxx

 - Steps
+
 1. Go to '...'


Please try making some change, adding spaces doesn't help

Uyadav207 · 2021-03-13T15:33:11Z

README.md

+
 ```sh
 npm i
 ```
+
 3.  Run the development server
+
 ```sh


only spaces are added to the file?

Uyadav207 · 2021-03-13T15:35:14Z

package.json

+    "postinstall": "patch-package",
+    "format": "prettier --write \"**/*.{js,jsx,json,md}\""


please avoid changing the packages.... (can cause merge conflict for others PR's)

Uyadav207 · 2021-03-13T15:35:34Z

public/manifest.json

      "type": "image/png",
      "sizes": "512x512"
-    }	    
+    }


space again ?

Uyadav207 · 2021-03-13T15:37:11Z

src/App.js

+import {
+  SafeAreaInsetsContext,
+  SafeAreaProvider,
+} from "react-native-safe-area-context";

 import Root from "./containers";
 import "./App.css";


This doesn't make any sense. only spaces have been added all over the place.

Uyadav207 · 2021-03-13T15:38:03Z

src/App.test.js

-import React from 'react';
-import ReactDOM from 'react-dom';
-import App from './App';
+import React from "react";
+import ReactDOM from "react-dom";
+import App from "./App";

-it('renders without crashing', () => {
-  const div = document.createElement('div');


replacements " " to ' ' and vice-a-versa.

Uyadav207 · 2021-03-13T15:38:48Z

src/index.js

+import React from "react";
+import ReactDOM from "react-dom";
+import "./index.css";
+import App from "./App";
+import * as serviceWorker from "./serviceWorker";
+import "react-native-safe-area-context";

-ReactDOM.render(<App />, document.getElementById('root'));
+ReactDOM.render(<App />, document.getElementById("root"));



This change will create conflict for other PR's

Uyadav207 · 2021-03-13T15:39:47Z

src/serviceWorker.js

+    .catch((error) => {
+      console.error("Error during service worker registration:", error);
    });
 }

 function checkValidServiceWorker(swUrl, config) {
  // Check if the service worker can be found. If it can't reload the page.
  fetch(swUrl)
-    .then(response => {
+    .then((response) => {
      // Ensure service worker exists, and that we really are getting a JS file.
-      const contentType = response.headers.get('content-type');
+      const contentType = response.headers.get("content-type");
      if (
        response.status === 404 ||
-        (contentType != null && contentType.indexOf('javascript') === -1)
+        (contentType != null && contentType.indexOf("javascript") === -1)
      ) {
        // No service worker found. Probably a different app. Reload the page.
-        navigator.serviceWorker.ready.then(registration => {
+        navigator.serviceWorker.ready.then((registration) => {
          registration.unregister().then(() => {
            window.location.reload();
          });


replacements " " to ' ' and vice-a-versa.

Uyadav207 · 2021-03-13T15:40:06Z

src/serviceWorker.js

+        "No internet connection found. App is running in offline mode."
      );
    });
 }

 export function unregister() {
-  if ('serviceWorker' in navigator) {
-    navigator.serviceWorker.ready.then(registration => {
+  if ("serviceWorker" in navigator) {
+    navigator.serviceWorker.ready.then((registration) => {


replacements " " to ' ' and vice-a-versa.

Uyadav207 · 2021-03-13T15:42:25Z

@abhishekkumar08, please have a look at ......

https://reactnativeelements.com/docs/contributing

abhishekkumar08 · 2021-03-13T16:17:13Z

@abhishekkumar08, please have a look at ......
* https://reactnativeelements.com/docs/contributing

If you try to understand the PR, it adds prettier to the code base which is a code formatter, I just installed it as a dev dependency and ran the command for it to format the whole code base. Thanks for the review. @Uyadav207

Uyadav207 · 2021-03-13T16:25:38Z

@abhishekkumar08, ok if it's prettier also... the changes made are quite huge and this will create merge conflict... for the previous PR, as I have faced the same problem yesterday. 😢 and had a hard time resolving conflict.

You can check 🔥 ( Hot-Fix )(resolved-conflict): Rectification of code by removing all (jsx-A11y) warnings (Conflict Resolved) #47 made a hot-fix for the conflict.
Try not to do this.. it causes syncing the branches and keeping them updated with upstream difficult. (Talking from experience).

I hope you understand? 😊

abhishekkumar08 · 2021-03-13T16:30:20Z

@Uyadav207 Yeah bro I got your point 😄 . Thanks for the suggestion, any other way how should I do this change?

tewarig

but issue #56 #57 and pr made on these are almost the same adding prettier as a dev dependency is not a good idea. we need to run npm run format every time before we make a pr?
in #56 we are adding a file... which will automatically format the code in vs code if the prettier
the extension is installed.

tewarig · 2021-03-13T16:55:09Z

@Uyadav207 Yeah bro I got your point. Thanks for the suggestion, any other way how should I do this change?

just keep an eye on the repo if any pr is merged in this repo make sure you update your repo and format its code again

Uyadav207 · 2021-03-13T16:57:27Z

but issue #56 #57 and pr made on these are almost the same adding prettier as a dev dependency is not a good idea. we need to run npm run format every time before we make a pr?

in #56 we are adding a file... which will automatically format the code in vs code if the prettier

the extension is installed.

Exactly.

Uyadav207 · 2021-03-13T17:01:41Z

@Uyadav207 Yeah bro I got your point 😄 . Thanks for the suggestion, any other way how should I do this change?

This is a breaking change. Need to wait for all PR to get merge than if we push your change.... Then it would not cause any conflict... but syncing the fork then would be necessary for all contributors... Is this making any sense ?

jugshaurya · 2021-03-13T17:07:41Z

@abhishekkumar08 already created an issue and made a PR about the same thing earlier than you. Kindly work on some other issues.

jugshaurya · 2021-03-13T17:09:50Z

@Uyadav207 Yeah bro I got your point smile . Thanks for the suggestion, any other way how should I do this change?

This is a breaking change. Need to wait for all PR to get merge than if we push your change.... Then it would not cause any conflict... but syncing the fork then would be necessary for all contributors... Is this making any sense ?

@Uyadav207 Initially it may seem a conflicting change but it will be beneficial in long go as developers contributing to the project will be following same styling.

tewarig · 2021-03-13T17:13:46Z

@Uyadav207 Yeah bro I got your point smile . Thanks for the suggestion, any other way how should I do this change?

This is a breaking change. Need to wait for all PR to get merge than if we push your change.... Then it would not cause any conflict... but syncing the fork then would be necessary for all contributors... Is this making any sense ?

@Uyadav207 Initially it may seem a conflicting change but it will be beneficial in long go as developers contributing to the project will be following same styling.

yes it will help a lot .

feat:Adding prettier as a dev dependency and formatting the whole code

724040f

Fixes:react-native-elements#58

Uyadav207 reviewed Mar 13, 2021

View reviewed changes

tewarig reviewed Mar 13, 2021

View reviewed changes

abhishekkumar08 closed this Mar 13, 2021

		"postinstall": "patch-package",
		"format": "prettier --write \"*/.{js,jsx,json,md}\""

Conversation

abhishekkumar08 commented Mar 13, 2021

Uh oh!

netlify bot commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uyadav207 commented Mar 13, 2021

Uh oh!

abhishekkumar08 commented Mar 13, 2021

Uh oh!

Uyadav207 commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhishekkumar08 commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tewarig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tewarig commented Mar 13, 2021

Uh oh!

Uyadav207 commented Mar 13, 2021

Uh oh!

Uyadav207 commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jugshaurya commented Mar 13, 2021

Uh oh!

jugshaurya commented Mar 13, 2021

Uh oh!

tewarig commented Mar 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Mar 13, 2021 •

edited

Loading

Uyadav207 commented Mar 13, 2021 •

edited

Loading

abhishekkumar08 commented Mar 13, 2021 •

edited

Loading

tewarig left a comment •

edited

Loading

Uyadav207 commented Mar 13, 2021 •

edited

Loading