From 99e14e88c005eb09c1a7410880212ec151bb6a09 Mon Sep 17 00:00:00 2001 From: paean99 Date: Sat, 22 Dec 2018 17:47:48 +0000 Subject: [PATCH 01/10] added prism rendeder --- packages/web/components/CodeFile.tsx | 66 ++++--- packages/web/package.json | 1 + packages/web/types/prism-react-renderer.ts | 194 ++++++++++++++++++++- 3 files changed, 237 insertions(+), 24 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index ed51819..73f5000 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -1,15 +1,13 @@ -import { useRef } from "react"; -import * as Prism from "prismjs"; +//import * as Prism from "prismjs"; import "prismjs/themes/prism.css"; import "prismjs/themes/prism-coy.css"; -import "prismjs/plugins/line-numbers/prism-line-numbers.css"; -import "prismjs/plugins/line-numbers/prism-line-numbers.js"; -import "prismjs/plugins/line-highlight/prism-line-highlight.css"; -import "prismjs/plugins/line-highlight/prism-line-highlight.js"; + +import styled from "styled-components"; +import Highlight, { defaultProps, Language } from "prism-react-renderer"; +//import theme from "prism-react-renderer/themes/vsDarkPlus"; import { FindCodeReviewQuestionsComponent } from "./apollo-components"; import { filenameToLang } from "../utils/filenameToLang"; -import { loadLanguage } from "../utils/loadLanguage"; import { QuestionSection } from "./QuestionSection"; interface Props { @@ -19,9 +17,25 @@ interface Props { } export const CodeFile: React.SFC = ({ code, path, postId }) => { - const hasLoadedLanguage = useRef(false); + const Pre = styled.pre` + text-align: left; + margin: 1em 0; + padding: 0.5em; + + & .token-line { + line-height: 1.3em; + height: 1.3em; + } + `; + + const LineNo = styled.span` + display: inline-block; + width: 2em; + user-select: none; + opacity: 0.3; + `; - const lang = path ? filenameToLang(path) : ""; + const lang: Language = path ? filenameToLang(path) : ""; const variables = { path, postId, @@ -40,21 +54,27 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { return ( <> - 
 {
-                if (!hasLoadedLanguage.current) {
-                  try {
-                    await loadLanguage(lang);
-                  } catch {}
-                  Prism.highlightAll();
-                  hasLoadedLanguage.current = true;
-                }
-              }}
-              className="line-numbers"
-              data-line={dataLines.join(" ")}
+            
-              {code}
-
+ {({ className, style, tokens, getLineProps, getTokenProps }) => ( + 
+                  {tokens.map((line, i) => {
+                    return (
+                      

+                        {i + 1}
+                        {line.map((token, key) => {
+                          return ;
+                        })}
+                      

+                    );
+                  })}
+
+ )} + PrismToken[] | string[]; + highlight: ( + code: string, + grammar: PrismGrammar, + language: Language + ) => string; + }; + + type PrismThemeEntry = { + color?: string; + backgroundColor?: string; + fontStyle?: "normal" | "italic"; + fontWeight?: + | "normal" + | "bold" + | "100" + | "200" + | "300" + | "400" + | "500" + | "600" + | "700" + | "800" + | "900"; + textDecorationLine?: + | "none" + | "underline" + | "line-through" + | "underline line-through"; + opacity?: number; + [styleKey: string]: string | number | void; + }; + + type PrismTheme = { + plain: PrismThemeEntry; + styles: Array<{ + types: string[]; + style: PrismThemeEntry; + languages?: Language[]; + }>; + }; + + type ThemeDict = { + root: StyleObj; + plain: StyleObj; + [type: string]: StyleObj; + }; + + type Token = { + types: string[]; + content: string; + empty?: boolean; + }; + + type PrismToken = { + type: string; + content: Array | string; + }; + + type StyleObj = { + [key: string]: string | number | null; + }; + + type LineInputProps = { + key?: React.Key; + style?: StyleObj; + className?: string; + line: Token[]; + [otherProp: string]: any; + }; + + type LineOutputProps = { + key?: React.Key; + style?: StyleObj; + className: string; + [otherProps: string]: any; + }; + + type TokenInputProps = { + key?: React.Key; + style?: StyleObj; + className?: string; + token: Token; + [otherProp: string]: any; + }; + + type TokenOutputProps = { + key?: React.Key; + style?: StyleObj; + className: string; + children: string; + [otherProp: string]: any; + }; + + type RenderProps = { + tokens: Token[][]; + className: string; + style: StyleObj; + getLineProps: (input: LineInputProps) => LineOutputProps; + getTokenProps: (input: TokenInputProps) => TokenOutputProps; + }; + + type DefaultProps = { + Prism: PrismLib; + theme: PrismTheme; + }; + + interface HighlightProps { + Prism: PrismLib; + theme?: PrismTheme; + language: Language; + code: string; + children: (props: RenderProps) => React.ReactNode; + } + + export default class Highlight extends React.Component { + themeDict: ThemeDict; + getLineProps: (lineInputProps: LineInputProps) => LineOutputProps; + getStyleForToken: (token: Token) => { [inlineStyle: string]: string }; + getTokenProps: (tokenInputPropsL: TokenInputProps) => TokenOutputProps; + } + + export const defaultProps: DefaultProps; + + export const Prism: PrismLib; + + export { Language, DefaultProps, PrismTheme }; +} + +declare module "prism-react-renderer/themes/*" { + import { PrismTheme } from "prism-react-renderer"; + const theme: PrismTheme; + export default theme; +} From 76436a7cb86298c2864bbd784b46f9c1a7ecd7ba Mon Sep 17 00:00:00 2001 From: paean99 Date: Sat, 22 Dec 2018 20:04:04 +0000 Subject: [PATCH 02/10] corrected typo for styled component --- packages/web/components/CodeFile.tsx | 42 +++++++++++++++------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index 73f5000..5c3e906 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -1,5 +1,5 @@ //import * as Prism from "prismjs"; -import "prismjs/themes/prism.css"; +//import "prismjs/themes/prism.css"; import "prismjs/themes/prism-coy.css"; import styled from "styled-components"; @@ -16,25 +16,29 @@ interface Props { postId: string; } -export const CodeFile: React.SFC = ({ code, path, postId }) => { - const Pre = styled.pre` - text-align: left; - margin: 1em 0; - padding: 0.5em; +const Pre = styled.pre` + text-align: left; + margin: 4em 0; + padding: 0.5em; + + & .token-line { + line-height: 1.3em; + height: 1.3em; + } - & .token-line { - line-height: 1.3em; - height: 1.3em; - } - `; + & .token-line:nth-child(odd) { + background: #f3faff; + } +`; - const LineNo = styled.span` - display: inline-block; - width: 2em; - user-select: none; - opacity: 0.3; - `; +const LineNo = styled.span` + display: inline-block; + width: 2em; + user-select: none; + opacity: 0.3; +`; +export const CodeFile: React.SFC = ({ code, path, postId }) => { const lang: Language = path ? filenameToLang(path) : ""; const variables = { path, @@ -61,7 +65,7 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { language={lang} > {({ className, style, tokens, getLineProps, getTokenProps }) => ( - 
+                
                   {tokens.map((line, i) => {
                     return (
                       

@@ -72,7 +76,7 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => {
                       

                     );
                   })}
-                

+
)} Date: Sat, 22 Dec 2018 21:23:34 +0000 Subject: [PATCH 03/10] Added highlight using Styled Components --- packages/web/components/CodeFile.tsx | 67 +++++++++++++++++----------- 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index 5c3e906..bca78bf 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -2,11 +2,14 @@ //import "prismjs/themes/prism.css"; import "prismjs/themes/prism-coy.css"; -import styled from "styled-components"; +import styled, { css } from "styled-components"; import Highlight, { defaultProps, Language } from "prism-react-renderer"; //import theme from "prism-react-renderer/themes/vsDarkPlus"; -import { FindCodeReviewQuestionsComponent } from "./apollo-components"; +import { + FindCodeReviewQuestionsComponent, + FindCodeReviewQuestionsQuery, +} from "./apollo-components"; import { filenameToLang } from "../utils/filenameToLang"; import { QuestionSection } from "./QuestionSection"; @@ -16,27 +19,21 @@ interface Props { postId: string; } -const Pre = styled.pre` - text-align: left; - margin: 4em 0; - padding: 0.5em; +const SelectLines = (prop: FindCodeReviewQuestionsQuery) => { + const styles = prop.findCodeReviewQuestions.reduce((total, current) => { + return (total += ` + & .token-line:nth-child(n+${current.startingLineNum}):nth-child(-n+${ + current.endingLineNum + }) { + background-color: #ffffcc; + } + `); + }, ""); - & .token-line { - line-height: 1.3em; - height: 1.3em; - } - - & .token-line:nth-child(odd) { - background: #f3faff; - } -`; - -const LineNo = styled.span` - display: inline-block; - width: 2em; - user-select: none; - opacity: 0.3; -`; + return css` + ${styles} + `; +}; export const CodeFile: React.SFC = ({ code, path, postId }) => { const lang: Language = path ? filenameToLang(path) : ""; @@ -52,9 +49,29 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { return null; } - const dataLines = data.findCodeReviewQuestions.map(q => { - return `${q.startingLineNum}-${q.endingLineNum}`; - }); + const Pre = styled.pre` + text-align: left; + margin: 4em 0; + padding: 0.5em; + + & .token-line { + line-height: 1.3em; + height: 1.3em; + } + + & .token-line:nth-child(odd) { + background: #f3faff; + } + + ${SelectLines(data)}; + `; + + const LineNo = styled.span` + display: inline-block; + width: 2em; + user-select: none; + opacity: 0.3; + `; return ( <> From 9b69610173af2cb17b7916a047afd96346c4b27c Mon Sep 17 00:00:00 2001 From: paean99 Date: Mon, 24 Dec 2018 00:58:35 +0000 Subject: [PATCH 04/10] Added mouse interaction to select lines --- packages/web/components/CodeFile.tsx | 86 ++++++++++++++++++++- packages/web/components/QuestionForm.tsx | 26 +++++-- packages/web/components/QuestionSection.tsx | 3 + packages/web/utils/useInputValue.ts | 11 ++- 4 files changed, 119 insertions(+), 7 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index bca78bf..655bd9e 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -1,3 +1,4 @@ +import { useState } from "react"; //import * as Prism from "prismjs"; //import "prismjs/themes/prism.css"; import "prismjs/themes/prism-coy.css"; @@ -19,6 +20,12 @@ interface Props { postId: string; } +/* + * *Styles for the line numbers coming from the server + * + * TODO: Perhaps refactor SelectLinesMouse as a 'sub function' of SelectLines? + * Or the two in a more general utils? + */ const SelectLines = (prop: FindCodeReviewQuestionsQuery) => { const styles = prop.findCodeReviewQuestions.reduce((total, current) => { return (total += ` @@ -29,19 +36,83 @@ const SelectLines = (prop: FindCodeReviewQuestionsQuery) => { } `); }, ""); + return css` + ${styles} + `; +}; + +/* + *Styles for the onClick line numbers + * + * TODO: Perhaps refactor SelectLinesMouse as a 'sub function' of SelectLines? + * Or the two in a more general utils? + */ +const SelectLinesMouse = (arg: number[]) => { + // establishing defaults + // The lenght of the args array can be variable + const startLine = arg[0] || 0; + const endLine = arg[1] || startLine; + const styles = ` + & .token-line:nth-child(n+${startLine}):nth-child(-n+${endLine}) { + background-color: #ffddbb; + } + `; return css` ${styles} `; }; export const CodeFile: React.SFC = ({ code, path, postId }) => { + const [lineSelectionState, setLineSelectionState] = useState([]); const lang: Language = path ? filenameToLang(path) : ""; const variables = { path, postId, }; + // Handler to manage the array of selected lines + const handleSelectLine = (lineNumber: number) => { + const tempSelectionState = [...lineSelectionState]; + + if (tempSelectionState.length == 0) { + tempSelectionState.push(lineNumber); + tempSelectionState.sort((a, b) => a - b); + setLineSelectionState([...tempSelectionState]); + return; + } + + const lineExist = tempSelectionState + .filter(value => { + return value !== lineNumber; + }) + .sort((a, b) => a - b); + + if (lineExist.length !== tempSelectionState.length) { + setLineSelectionState([...lineExist]); + return; + } + + if (tempSelectionState.length == 2) { + tempSelectionState[1] = lineNumber; + tempSelectionState.sort((a, b) => a - b); + setLineSelectionState([...tempSelectionState]); + return; + } + + if (tempSelectionState.length > 0 || tempSelectionState.length < 2) { + tempSelectionState.push(lineNumber); + tempSelectionState.sort((a, b) => a - b); + setLineSelectionState([...tempSelectionState]); + return; + } + + // perhaps not really needed, but just for completeness sake... + tempSelectionState.sort((a, b) => a - b); + setLineSelectionState([...tempSelectionState]); + return; + }; + return ( {({ data, loading }) => { @@ -59,18 +130,27 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { height: 1.3em; } + /* Style for the effect of alternating colors in the background */ & .token-line:nth-child(odd) { background: #f3faff; } ${SelectLines(data)}; + + ${SelectLinesMouse(lineSelectionState)}; `; + /* Style for the line numbers */ const LineNo = styled.span` display: inline-block; width: 2em; user-select: none; opacity: 0.3; + &:hover { + font-weight: 900; + opacity: 0.4; + cursor: pointer; + } `; return ( @@ -86,7 +166,9 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { {tokens.map((line, i) => { return (
- {i + 1} + handleSelectLine(i + 1)}> + {i + 1} + {line.map((token, key) => { return ; })} @@ -102,6 +184,8 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { postId={postId} programmingLanguage={lang} path={path} + /* Added a props to pass the selected lines */ + linesSelection={lineSelectionState} /> ); diff --git a/packages/web/components/QuestionForm.tsx b/packages/web/components/QuestionForm.tsx index 8eb6da4..69f5b9b 100644 --- a/packages/web/components/QuestionForm.tsx +++ b/packages/web/components/QuestionForm.tsx @@ -1,4 +1,5 @@ import * as React from "react"; +import { useEffect } from "react"; import { CreateCodeReviewQuestionComponent } from "./apollo-components"; import { useInputValue } from "../utils/useInputValue"; @@ -8,6 +9,7 @@ export interface QuestionFormProps { path?: string; postId: string; programmingLanguage?: string; + linesSelection: number[]; } export const QuestionForm = ({ @@ -15,11 +17,25 @@ export const QuestionForm = ({ path, postId, programmingLanguage, + linesSelection, }: QuestionFormProps) => { - const [startingLineNum, startingLineNumChange] = useInputValue("0"); - const [endingLineNum, endingLineNumChange] = useInputValue("0"); + const [startingLineNum, setStartingLineNum] = useInputValue("0"); + const [endingLineNum, setEndingLineNum] = useInputValue("0"); const [text, textChange] = useInputValue(""); + useEffect( + () => { + // Used local constant + // but the values may be passed directly with the || ? + const startLinesSelection = linesSelection[0] || 0; + const endLinesSelection = linesSelection[1] || 0; + + setStartingLineNum(startLinesSelection); + setEndingLineNum(endLinesSelection); + }, + [linesSelection] + ); + return ( {mutate => ( @@ -47,20 +63,20 @@ export const QuestionForm = ({ }, }); - console.log(response); + //console.log(response); }} > { return ( @@ -31,6 +33,7 @@ export const QuestionSection = ({ postId={postId} path={path} programmingLanguage={programmingLanguage} + linesSelection={linesSelection} />
{data.findCodeReviewQuestions.map(crq => ( diff --git a/packages/web/utils/useInputValue.ts b/packages/web/utils/useInputValue.ts index a32b304..5efb143 100644 --- a/packages/web/utils/useInputValue.ts +++ b/packages/web/utils/useInputValue.ts @@ -3,7 +3,16 @@ import { useState, useCallback } from "react"; export function useInputValue(initialValue: T): [T, (e: any) => void] { const [value, setValue] = useState(initialValue); const onChange = useCallback(event => { - setValue(event.currentTarget.value); + // TODO: needs heavy refactoring + // dealing with numbers and events as one... + // it works but it is a bad pattern and ugly code + if (typeof event == "number") { + // maybe event.toString. + // It works without it as the input must be converting it? + setValue(event); + } else { + setValue(event.currentTarget.value); + } }, []); return [value, onChange]; From 00555e42433d9f8e0fb30bf370dd6d1e520f23e5 Mon Sep 17 00:00:00 2001 From: paean99 Date: Mon, 24 Dec 2018 19:24:21 +0000 Subject: [PATCH 05/10] put React Hooks outside of conditions --- packages/web/components/CodeFile.tsx | 40 +++++++++++----------------- packages/web/utils/useInputValue.ts | 24 ++++++++++++++--- 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index 655bd9e..83f6509 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -71,16 +71,12 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { postId, }; - // Handler to manage the array of selected lines + /* + * Handler to manage the array of selected lines + * It simulates the github line number selection + * */ const handleSelectLine = (lineNumber: number) => { - const tempSelectionState = [...lineSelectionState]; - - if (tempSelectionState.length == 0) { - tempSelectionState.push(lineNumber); - tempSelectionState.sort((a, b) => a - b); - setLineSelectionState([...tempSelectionState]); - return; - } + let tempSelectionState = [...lineSelectionState]; const lineExist = tempSelectionState .filter(value => { @@ -88,26 +84,20 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { }) .sort((a, b) => a - b); - if (lineExist.length !== tempSelectionState.length) { - setLineSelectionState([...lineExist]); - return; - } - - if (tempSelectionState.length == 2) { + // so many if else are not so legible... + // a switch might be possible here, but does it bring more or less? + if (tempSelectionState.length == 0) { + tempSelectionState.push(lineNumber); + } else if (lineExist.length !== tempSelectionState.length) { + tempSelectionState = [...lineExist]; + } else if (tempSelectionState.length == 2) { tempSelectionState[1] = lineNumber; - tempSelectionState.sort((a, b) => a - b); - setLineSelectionState([...tempSelectionState]); - return; - } - - if (tempSelectionState.length > 0 || tempSelectionState.length < 2) { + } else if (tempSelectionState.length > 0 || tempSelectionState.length < 2) { + // this is the same as the first condition, but the order is important... tempSelectionState.push(lineNumber); - tempSelectionState.sort((a, b) => a - b); - setLineSelectionState([...tempSelectionState]); - return; } - // perhaps not really needed, but just for completeness sake... + // The react hook must be outside conditions? tempSelectionState.sort((a, b) => a - b); setLineSelectionState([...tempSelectionState]); return; diff --git a/packages/web/utils/useInputValue.ts b/packages/web/utils/useInputValue.ts index 5efb143..f6739b6 100644 --- a/packages/web/utils/useInputValue.ts +++ b/packages/web/utils/useInputValue.ts @@ -6,13 +6,31 @@ export function useInputValue(initialValue: T): [T, (e: any) => void] { // TODO: needs heavy refactoring // dealing with numbers and events as one... // it works but it is a bad pattern and ugly code + + /* + * Probably best to use an object in the args + * arg: { + * eventType: string // can be "number" or "MouseEvent" + * eventMouse: MouseEvent + * eventNumber: Number + * } + * Then we can just have a condition to check the eventType + * and setValue appropriately + * + * This would be more complex but it would "save" the type system + * + * */ + + let tempValue: any = null; if (typeof event == "number") { // maybe event.toString. - // It works without it as the input must be converting it? - setValue(event); + // It works without it as the input element must be converting it? + tempValue = event; } else { - setValue(event.currentTarget.value); + tempValue = event.currentTarget.value; } + // react hooks outside of conditions? + setValue(tempValue); }, []); return [value, onChange]; From 82f8d83a1748f9d879d898542b719b1c2a83008f Mon Sep 17 00:00:00 2001 From: paean99 Date: Wed, 26 Dec 2018 13:06:01 +0000 Subject: [PATCH 06/10] small corrections --- packages/web/components/CodeFile.tsx | 8 +++----- packages/web/components/QuestionForm.tsx | 4 ++-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index 83f6509..08c0c34 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -78,11 +78,9 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { const handleSelectLine = (lineNumber: number) => { let tempSelectionState = [...lineSelectionState]; - const lineExist = tempSelectionState - .filter(value => { - return value !== lineNumber; - }) - .sort((a, b) => a - b); + const lineExist = tempSelectionState.filter(value => { + return value !== lineNumber; + }); // so many if else are not so legible... // a switch might be possible here, but does it bring more or less? diff --git a/packages/web/components/QuestionForm.tsx b/packages/web/components/QuestionForm.tsx index 69f5b9b..e1a67ad 100644 --- a/packages/web/components/QuestionForm.tsx +++ b/packages/web/components/QuestionForm.tsx @@ -27,8 +27,8 @@ export const QuestionForm = ({ () => { // Used local constant // but the values may be passed directly with the || ? - const startLinesSelection = linesSelection[0] || 0; - const endLinesSelection = linesSelection[1] || 0; + const startLinesSelection = linesSelection ? linesSelection[0] || 0 : 0; + const endLinesSelection = linesSelection ? linesSelection[1] || 0 : 0; setStartingLineNum(startLinesSelection); setEndingLineNum(endLinesSelection); From 0e7e92fa2b56481f4579fffb4e477a7686d4c986 Mon Sep 17 00:00:00 2001 From: paean99 Date: Wed, 26 Dec 2018 15:59:33 +0000 Subject: [PATCH 07/10] lineSelectionState no longer undefined on startup --- packages/web/components/CodeFile.tsx | 30 ++++++++++++++---------- packages/web/components/QuestionForm.tsx | 4 ++-- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index 08c0c34..cb5b095 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -64,7 +64,10 @@ const SelectLinesMouse = (arg: number[]) => { }; export const CodeFile: React.SFC = ({ code, path, postId }) => { - const [lineSelectionState, setLineSelectionState] = useState([]); + const [lineSelectionState, setLineSelectionState] = useState([ + 0, + 0, + ]); const lang: Language = path ? filenameToLang(path) : ""; const variables = { path, @@ -76,25 +79,28 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { * It simulates the github line number selection * */ const handleSelectLine = (lineNumber: number) => { - let tempSelectionState = [...lineSelectionState]; + let tempSelectionState = lineSelectionState.filter(value => { + return value !== 0; + }); - const lineExist = tempSelectionState.filter(value => { + const withoutRepeatLines = tempSelectionState.filter(value => { return value !== lineNumber; }); - // so many if else are not so legible... + // so many else/if are not so legible... // a switch might be possible here, but does it bring more or less? if (tempSelectionState.length == 0) { - tempSelectionState.push(lineNumber); - } else if (lineExist.length !== tempSelectionState.length) { - tempSelectionState = [...lineExist]; - } else if (tempSelectionState.length == 2) { + tempSelectionState = [lineNumber, lineNumber]; + } else if (withoutRepeatLines.length == 0) { + tempSelectionState = [0, 0]; + } else if (tempSelectionState.length !== withoutRepeatLines.length) { + tempSelectionState = [withoutRepeatLines[0], withoutRepeatLines[0]]; + } else if ( + tempSelectionState.length == 2 || + tempSelectionState.length == 1 + ) { tempSelectionState[1] = lineNumber; - } else if (tempSelectionState.length > 0 || tempSelectionState.length < 2) { - // this is the same as the first condition, but the order is important... - tempSelectionState.push(lineNumber); } - // The react hook must be outside conditions? tempSelectionState.sort((a, b) => a - b); setLineSelectionState([...tempSelectionState]); diff --git a/packages/web/components/QuestionForm.tsx b/packages/web/components/QuestionForm.tsx index e1a67ad..0af0197 100644 --- a/packages/web/components/QuestionForm.tsx +++ b/packages/web/components/QuestionForm.tsx @@ -25,8 +25,8 @@ export const QuestionForm = ({ useEffect( () => { - // Used local constant - // but the values may be passed directly with the || ? + // the undefined check is probably no longer needed + // but an extra check, in this type of app, are never a negative thing? const startLinesSelection = linesSelection ? linesSelection[0] || 0 : 0; const endLinesSelection = linesSelection ? linesSelection[1] || 0 : 0; From 1ff643e26eab2631bab6818ae58677be7478ff41 Mon Sep 17 00:00:00 2001 From: paean99 Date: Wed, 26 Dec 2018 20:22:59 +0000 Subject: [PATCH 08/10] temp solution to link the input back to lines selection --- packages/web/components/CodeFile.tsx | 29 +++++++++++++-- packages/web/components/QuestionForm.tsx | 39 ++++++++------------- packages/web/components/QuestionSection.tsx | 16 +++++++-- 3 files changed, 55 insertions(+), 29 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index cb5b095..c921fea 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -68,6 +68,8 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { 0, 0, ]); + const [startLinesSelection, setStartLinesSelection] = useState(0); + const [endLinesSelection, setEndLinesSelection] = useState(0); const lang: Language = path ? filenameToLang(path) : ""; const variables = { path, @@ -103,10 +105,30 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { } // The react hook must be outside conditions? tempSelectionState.sort((a, b) => a - b); + setStartLinesSelection(tempSelectionState[0] || 0); + setEndLinesSelection(tempSelectionState[1] || 0); setLineSelectionState([...tempSelectionState]); return; }; + /* + * handleStartLinesSelection and handleEndLinesSelection are temporary solutions + * They are still buggy + * TODO: add a 'debounceTime' to the inputs values for an easier UX experience + * it should be easy to do it or maybe just use something like + * https://www.npmjs.com/package/react-debounce-input or + * https://lodash.com/docs/#debounce (used by the above ref) + */ + const handleStartLinesSelection = (event: any) => { + setLineSelectionState([event.currentTarget.value, lineSelectionState[1]]); + setStartLinesSelection(event.currentTarget.value); + }; + + const handleEndLinesSelection = (event: any) => { + setLineSelectionState([lineSelectionState[0], event.currentTarget.value]); + setEndLinesSelection(event.currentTarget.value); + }; + return ( {({ data, loading }) => { @@ -178,8 +200,11 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { postId={postId} programmingLanguage={lang} path={path} - /* Added a props to pass the selected lines */ - linesSelection={lineSelectionState} + /* Added a props to pass and handle the selected lines */ + startLinesSelection={startLinesSelection} + endLinesSelection={endLinesSelection} + handleStartLinesSelection={handleStartLinesSelection} + handleEndLinesSelection={handleEndLinesSelection} /> ); diff --git a/packages/web/components/QuestionForm.tsx b/packages/web/components/QuestionForm.tsx index 0af0197..548e40f 100644 --- a/packages/web/components/QuestionForm.tsx +++ b/packages/web/components/QuestionForm.tsx @@ -1,5 +1,5 @@ import * as React from "react"; -import { useEffect } from "react"; +import { ChangeEvent } from "react"; import { CreateCodeReviewQuestionComponent } from "./apollo-components"; import { useInputValue } from "../utils/useInputValue"; @@ -9,7 +9,10 @@ export interface QuestionFormProps { path?: string; postId: string; programmingLanguage?: string; - linesSelection: number[]; + startLinesSelection: number; + endLinesSelection: number; + handleStartLinesSelection: (event: ChangeEvent) => void; + handleEndLinesSelection: (event: ChangeEvent) => void; } export const QuestionForm = ({ @@ -17,33 +20,21 @@ export const QuestionForm = ({ path, postId, programmingLanguage, - linesSelection, + startLinesSelection, + endLinesSelection, + handleStartLinesSelection, + handleEndLinesSelection, }: QuestionFormProps) => { - const [startingLineNum, setStartingLineNum] = useInputValue("0"); - const [endingLineNum, setEndingLineNum] = useInputValue("0"); const [text, textChange] = useInputValue(""); - useEffect( - () => { - // the undefined check is probably no longer needed - // but an extra check, in this type of app, are never a negative thing? - const startLinesSelection = linesSelection ? linesSelection[0] || 0 : 0; - const endLinesSelection = linesSelection ? linesSelection[1] || 0 : 0; - - setStartingLineNum(startLinesSelection); - setEndingLineNum(endLinesSelection); - }, - [linesSelection] - ); - return ( {mutate => (
{ e.preventDefault(); - const start = parseInt(startingLineNum, 10); - const end = parseInt(endingLineNum, 10); + const start = startLinesSelection; + const end = endLinesSelection; const response = await mutate({ variables: { codeReviewQuestion: { @@ -69,14 +60,14 @@ export const QuestionForm = ({ ) => void; + handleEndLinesSelection: (event: ChangeEvent) => void; } export const QuestionSection = ({ @@ -17,7 +21,10 @@ export const QuestionSection = ({ postId, path, programmingLanguage, - linesSelection, + startLinesSelection, + endLinesSelection, + handleStartLinesSelection, + handleEndLinesSelection, }: Props) => { return ( @@ -33,7 +40,10 @@ export const QuestionSection = ({ postId={postId} path={path} programmingLanguage={programmingLanguage} - linesSelection={linesSelection} + startLinesSelection={startLinesSelection} + endLinesSelection={endLinesSelection} + handleStartLinesSelection={handleStartLinesSelection} + handleEndLinesSelection={handleEndLinesSelection} />
{data.findCodeReviewQuestions.map(crq => ( From 2d53707cb9ea4d86e01c9f7fca0865e653236095 Mon Sep 17 00:00:00 2001 From: paean99 Date: Thu, 27 Dec 2018 03:21:33 +0000 Subject: [PATCH 09/10] added react-debounce-input for better UX --- packages/web/components/CodeFile.tsx | 39 +++++++++++++++++------- packages/web/components/QuestionForm.tsx | 17 +++++++---- packages/web/package.json | 1 + 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/packages/web/components/CodeFile.tsx b/packages/web/components/CodeFile.tsx index c921fea..0ff0562 100644 --- a/packages/web/components/CodeFile.tsx +++ b/packages/web/components/CodeFile.tsx @@ -1,4 +1,4 @@ -import { useState } from "react"; +import { useState, useEffect } from "react"; //import * as Prism from "prismjs"; //import "prismjs/themes/prism.css"; import "prismjs/themes/prism-coy.css"; @@ -64,12 +64,14 @@ const SelectLinesMouse = (arg: number[]) => { }; export const CodeFile: React.SFC = ({ code, path, postId }) => { + // does this needs so many states? Should be simplified more... const [lineSelectionState, setLineSelectionState] = useState([ 0, 0, ]); const [startLinesSelection, setStartLinesSelection] = useState(0); const [endLinesSelection, setEndLinesSelection] = useState(0); + const lang: Language = path ? filenameToLang(path) : ""; const variables = { path, @@ -112,21 +114,36 @@ export const CodeFile: React.SFC = ({ code, path, postId }) => { }; /* - * handleStartLinesSelection and handleEndLinesSelection are temporary solutions - * They are still buggy - * TODO: add a 'debounceTime' to the inputs values for an easier UX experience - * it should be easy to do it or maybe just use something like - * https://www.npmjs.com/package/react-debounce-input or - * https://lodash.com/docs/#debounce (used by the above ref) + * handleStartLinesSelection and handleEndLinesSelection + * are still a little buggy, but almost there + * TODO: merge the 2 functions to avoid repetition + * TODO: better handling of the situation when values of input1 > input2 + * TODO: ability to reset the line selections directly from the input */ const handleStartLinesSelection = (event: any) => { - setLineSelectionState([event.currentTarget.value, lineSelectionState[1]]); - setStartLinesSelection(event.currentTarget.value); + if (event.target) { + // || lineSelectionState[0] to block NaN and secure default + // TODO: add a better checks + let value1 = parseInt(event.target.value, 10) || lineSelectionState[0]; + let value2 = lineSelectionState[1]; + if (value1 > value2) { + value2 = value1; + } + setLineSelectionState([value1, value2]); + setStartLinesSelection(value1); + setEndLinesSelection(value2); + } }; const handleEndLinesSelection = (event: any) => { - setLineSelectionState([lineSelectionState[0], event.currentTarget.value]); - setEndLinesSelection(event.currentTarget.value); + if (event.target) { + // || lineSelectionState[0] to block NaN and secure default + // TODO: add a better checks + let value = parseInt(event.target.value, 10) || lineSelectionState[1]; + value = value >= lineSelectionState[0] ? value : lineSelectionState[0]; + setLineSelectionState([lineSelectionState[0], value]); + setEndLinesSelection(value); + } }; return ( diff --git a/packages/web/components/QuestionForm.tsx b/packages/web/components/QuestionForm.tsx index 548e40f..df34a81 100644 --- a/packages/web/components/QuestionForm.tsx +++ b/packages/web/components/QuestionForm.tsx @@ -1,6 +1,8 @@ import * as React from "react"; import { ChangeEvent } from "react"; +import { DebounceInput } from "react-debounce-input"; + import { CreateCodeReviewQuestionComponent } from "./apollo-components"; import { useInputValue } from "../utils/useInputValue"; @@ -57,16 +59,19 @@ export const QuestionForm = ({ //console.log(response); }} > - - Date: Thu, 27 Dec 2018 12:30:42 +0000 Subject: [PATCH 10/10] cleanup of useInputValues to the original form --- packages/web/utils/useInputValue.ts | 29 +---------------------------- 1 file changed, 1 insertion(+), 28 deletions(-) diff --git a/packages/web/utils/useInputValue.ts b/packages/web/utils/useInputValue.ts index f6739b6..a32b304 100644 --- a/packages/web/utils/useInputValue.ts +++ b/packages/web/utils/useInputValue.ts @@ -3,34 +3,7 @@ import { useState, useCallback } from "react"; export function useInputValue(initialValue: T): [T, (e: any) => void] { const [value, setValue] = useState(initialValue); const onChange = useCallback(event => { - // TODO: needs heavy refactoring - // dealing with numbers and events as one... - // it works but it is a bad pattern and ugly code - - /* - * Probably best to use an object in the args - * arg: { - * eventType: string // can be "number" or "MouseEvent" - * eventMouse: MouseEvent - * eventNumber: Number - * } - * Then we can just have a condition to check the eventType - * and setValue appropriately - * - * This would be more complex but it would "save" the type system - * - * */ - - let tempValue: any = null; - if (typeof event == "number") { - // maybe event.toString. - // It works without it as the input element must be converting it? - tempValue = event; - } else { - tempValue = event.currentTarget.value; - } - // react hooks outside of conditions? - setValue(tempValue); + setValue(event.currentTarget.value); }, []); return [value, onChange];