Skip to content

feature: added markdowns list#6

Open
dgrimaldi wants to merge 1 commit intomainfrom
feature/markdown
Open

feature: added markdowns list#6
dgrimaldi wants to merge 1 commit intomainfrom
feature/markdown

Conversation

@dgrimaldi
Copy link
Copy Markdown
Owner

No description provided.

Comment thread src/App.tsx
@@ -1,30 +1,29 @@
import React from 'react';
import React, {useEffect, useRef} from 'react';
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Do we need to import react?

Comment thread src/App.tsx

function App() {

import Markdown from "./components/MarkdownComp";
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Suggested change
import Markdown from "./components/MarkdownComp";


const MarkdownComp = memo(function MarkdownComp({text, title}: {text: string, title?: string | ""}) {

const converter = new showdown.Converter();
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

we don't use this variable. Do we need it?

const MarkdownComp = memo(function MarkdownComp({text, title}: {text: string, title?: string | ""}) {

const converter = new showdown.Converter();
const cleanHTML = sanitize(converter.makeHtml(text))
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

we don't use this variable. Do we need it?

Comment thread package.json
"@types/showdown": "^2.0.2",
"antd": "^5.7.3",
"d3": "^7.8.5",
"dompurify": "^3.0.6",
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

the library is not used

Comment thread package.json
"react-scripts": "5.0.1",
"remark-gfm": "^4.0.0",
"sass": "^1.69.0",
"showdown": "^2.1.0",
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

the library is not used

Comment thread package.json
"@testing-library/react": "^13.4.0",
"@testing-library/user-event": "^13.5.0",
"@types/d3": "^7.4.0",
"@types/dompurify": "^3.0.3",
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

the library is not used

Comment thread src/features/AboutMe.tsx
// }

const AboutMe = () => {
const [isModalOpen, setModalOpen] = useState(false)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Could we use a different name for set. Something like updateIsModalOpen?

Comment thread src/features/AboutMe.tsx
<div style={{display: "flex"}}>
<Modal
open={isModalOpen}
okButtonProps={{ style: { display: "none"} }}
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Can we move all the styles in appropriate file or in an external variable?

Comment thread src/features/AboutMe.tsx
width={2000}
>
<MarkdownComp text={markdownTextList[selectedMarkdown - 1]} />
<Pagination simple total={30} onChange={(page)=> setSelectedMarkdown(page)} />
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Can we externalize the variable? maybe not an hard-coded number but related to markdowns' number

@dgrimaldi dgrimaldi temporarily deployed to github-pages October 17, 2023 22:20 — with GitHub Pages Inactive
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