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

Migration docusaurus 2 #429

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ThakurKarthik
Copy link

Hello @yangshun , @endiliey review this pr when you got time.

I was working on migrating docusaurus of this repo to new V2 version

Deployed at https://thakurkarthik.github.io/redex/ for testing.

This pr on successful merge takes the docusaurus version to new v2 alpha for fbredex

This PR belongs to #426 and facebook/docusaurus#1834

Here are some screenshots of the new site.

Landing Page

Screenshot (155)

Docs Page

Screenshot (156)

FAQ Page

Screenshot (157)

"write-translations": "docusaurus-write-translations",
"version": "docusaurus-version",
"rename-version": "docusaurus-rename-version"
"examples": "docusaurus examples",

Choose a reason for hiding this comment

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

Suggested change
"examples": "docusaurus examples",

"examples": "docusaurus examples",
"start": "docusaurus start",
"build": "docusaurus build",
"publish-gh-pages": "docusaurus publish",

Choose a reason for hiding this comment

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

Suggested change
"publish-gh-pages": "docusaurus publish",

"start": "docusaurus start",
"build": "docusaurus build",
"publish-gh-pages": "docusaurus publish",
"write-translations": "docusaurus write-translations",

Choose a reason for hiding this comment

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

Suggested change
"write-translations": "docusaurus write-translations",

"build": "docusaurus build",
"publish-gh-pages": "docusaurus publish",
"write-translations": "docusaurus write-translations",
"version": "docusaurus version",

Choose a reason for hiding this comment

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

Suggested change
"version": "docusaurus version",

"publish-gh-pages": "docusaurus publish",
"write-translations": "docusaurus write-translations",
"version": "docusaurus version",
"rename-version": "docusaurus rename-version",

Choose a reason for hiding this comment

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

Suggested change
"rename-version": "docusaurus rename-version",


/* Custom fonts for website */
/*
fonts: {

Choose a reason for hiding this comment

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

delete it. why make it commented when its unused

* LICENSE file in the root directory of this source tree.
*/

// See https://docusaurus.io/docs/site-config for all the possible

Choose a reason for hiding this comment

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

Suggested change
// See https://docusaurus.io/docs/site-config for all the possible

// },

// Add custom scripts here that would be placed in <script> tags.
scripts: ["https://buttons.github.io/buttons.js"],

Choose a reason for hiding this comment

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

i dont think its needed now

Copy link
Author

Choose a reason for hiding this comment

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

So github stars won't be coming,in v1 it was present in footer https://fbredex.com/

Choose a reason for hiding this comment

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

Yep we don't plan to show it in the footer anymore. You may ignore that.

Copy link

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

The website looks great @ThakurKarthik! Thanks for deploying on your gh-pages so we can check it out. The only comments I have are regarding the Index page and CSS. Otherwise looks great!

@@ -0,0 +1,129 @@
/**

Choose a reason for hiding this comment

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

Please use 2 spaces (at the most, 4) for indentation. Recommend you install the Prettier plugin for your editor.

@@ -59,13 +66,17 @@
.button {

Choose a reason for hiding this comment

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

Remove these .button styles. Use the default by Docusaurus CSS. Refer to https://github.com/facebook/create-react-app/blob/master/docusaurus/website/src/pages/index.js as an example of the CSS classes available.

@@ -0,0 +1,72 @@
/**

This comment was marked as resolved.

<div className="docMainWrapper wrapper">
<div className="container padding-vert--lg padding-top--xl">
<div className="post">
<header className="postHeader">

Choose a reason for hiding this comment

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

@@ -0,0 +1,110 @@
/**

Choose a reason for hiding this comment

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

Just use the default Footer, don't need to swizzle.

Copy link
Author

Choose a reason for hiding this comment

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

unswizzling here just means to delete the Footer file right?

Choose a reason for hiding this comment

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

Yep you're right!

}
];

class Button extends React.Component {

Choose a reason for hiding this comment

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

Don't need a Button component. Just do

import Link from '@docusaurus/Link`;

...

<Link className="button button--outline" href={withBaseUrl('...')} />

Using our <Link> component will cause it to do a front end redirect instead of full-page redirect.

--ifm-color-primary-lightest: rgb(186, 224, 210);
}

/* your custom css */

@font-face {
font-family: 'Lato';

Choose a reason for hiding this comment

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

I don't think the Lato Google font is loaded at all...

Copy link
Author

Choose a reason for hiding this comment

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

should i remove it?

Choose a reason for hiding this comment

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

Yes I'd lean towards that. We won't need to write too much in custom.css as they apply globally. Page-specific CSS should be written in CSS modules.

Copy link
Author

Choose a reason for hiding this comment

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

@yangshun please check for the updated build here https://thakurkarthik.github.io/redex/ .In my latest commit i have moved styles to local module.In the custom.css only some colors and fonts are present,for the fonts i am not that much aware if they are working or not.Can you please check for that and signal me accordingly.Thanks for the reviews !

Copy link
Contributor

Choose a reason for hiding this comment

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

The fonts look different from those on the current fbredex.com...

@ThakurKarthik
Copy link
Author

@yangshun @endiliey i have pushed my code,can you make some time and review it Thanks !

@yangshun
Copy link

@ThakurKarthik I think there are conflicts now. Could you resolve them?

From Redex site could @int3 help? Or I can do the reviewing instead and preserve as much of v1 design and functionality as possible.

@ThakurKarthik
Copy link
Author

@yangshun i haved merged with latest master now there should be no conflicts.

@int3
Copy link
Contributor

int3 commented Oct 30, 2019

I'm not too picky... would be nice to keep some of the old button/grey background styling, but I don't think it matters too much. I'm not a designer anyway :p

If you can fix the fonts I'm happy to merge this

@yangshun
Copy link

yangshun commented Oct 30, 2019 via email

@int3
Copy link
Contributor

int3 commented Oct 31, 2019

Ah alright then. Standardizing the font makes sense. Thanks!

@yangshun
Copy link

yangshun commented Nov 3, 2019

Ref: facebook/docusaurus#1834

@yangshun
Copy link

yangshun commented Nov 3, 2019

@ThakurKarthik could you remove the font styling in the CSS file? Then I think it should be good.

--ifm-color-primary-lightest: rgb(248, 248, 248);
}

body {
Copy link

Choose a reason for hiding this comment

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

Remove this whole class

--ifm-color-primary-lighter: rgb(158, 211, 191);
--ifm-color-primary-lightest: rgb(186, 224, 210);
--ifm-color-secondary: #f9f9f9;
--ifm-color-primary-dark: rgb(216, 216, 216);
Copy link

Choose a reason for hiding this comment

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

Please just leave what's necessary.

@@ -40,14 +55,6 @@
font-style: normal;
}
Copy link
Author

Choose a reason for hiding this comment

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

@yangshun should i remove fonts from website/static/css/custom.css too?

Copy link

Choose a reason for hiding this comment

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

Yes please

Copy link
Author

Choose a reason for hiding this comment

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

@yangshun I have removed the fonts.

},
"devDependencies": {
"docusaurus": "^1.7.2"
"@docusaurus/core": "^2.0.0-alpha.27",
"@docusaurus/preset-classic": "^2.0.0-alpha.27",
Copy link

Choose a reason for hiding this comment

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

@ThakurKarthik can you please update Docusaurus to the latest version?

Copy link
Author

Choose a reason for hiding this comment

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

I have updated them to newer 2.0.0-alpha.34 version

@justinjhendrick
Copy link
Contributor

Is this pull request ready to merge now? Has all the feedback been incorporated? Are all the reviewers happy?

@ThakurKarthik
Copy link
Author

Hey @justinjhendrick not sure if it the code is working anymore as docusaurus has gone through major changes i guess !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants