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

FLUID-6506: Migrated from Docpad to 11ty for GSoC 2020 #56

Merged
merged 56 commits into from Aug 24, 2020

Conversation

sachin10101998
Copy link
Contributor

GSoC 2020 Project

Copy link
Member

@BlueSlug BlueSlug left a comment

Choose a reason for hiding this comment

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

Looking great. I'm excited to see this in action!

It needs a few changes before I'd be comfortable with it being merged in, but it's very close already.

.gitignore Show resolved Hide resolved
AUTHORS.md Show resolved Hide resolved
AUTHORS.md Outdated Show resolved Hide resolved
Gruntfile.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/filters/w3-date-filter.js Outdated Show resolved Hide resolved
src/posts/2007-05-04-Better_Web_Experiences.md Outdated Show resolved Hide resolved
Gruntfile.js Show resolved Hide resolved
src/projects.html Outdated Show resolved Hide resolved
src/projects.html Outdated Show resolved Hide resolved
@BlueSlug
Copy link
Member

I also noticed that the spacing of elements differs between the live version running at https://fluidproject.org/ and the version I built locally. It would be good to know what's going on there, as the output should be identical (except for UIO which has been updated). It's subtle, but would be more apparent if you open these two sites and switch back and forth between them.

The live (DocPad) version:
2020-05-28 fluidproject spacing difference - live site

The dev (11ty) version:
2020-05-28 fluidproject spacing difference - localhost

@jhung
Copy link
Member

jhung commented May 29, 2020

There's a bug where loading the root of the site (i.e. localhost:9778/) incorrectly sets News as the current navigation item, but should be Home.

image

Loading the site using localhost:9778/index.html does not cause this problem.

@jhung
Copy link
Member

jhung commented May 29, 2020

The Infusion directory src/lib/infusion is being tracked, but I wonder if this is necessary or desired since the contents of that directory is copied from ./node_modules/infusion/dist/. @BlueSlug thoughts?

@sachin10101998
Copy link
Contributor Author

sachin10101998 commented May 29, 2020

@jhung @BlueSlug I fixed that bug where the default page showed news as highlighted or selected page. I've also removed the package lock json file and pinned dependencies in package.json file. Can we merge it now?

README.md Outdated Show resolved Hide resolved
@BlueSlug
Copy link
Member

BlueSlug commented Jun 5, 2020

The Infusion directory src/lib/infusion is being tracked, but I wonder if this is necessary or desired since the contents of that directory is copied from ./node_modules/infusion/dist/. @BlueSlug thoughts?

If Infusion is being pulled in via npm, then I agree that it isn't necessary to track this directory unless it's required for the build process.

package.json Outdated Show resolved Hide resolved
Copy link
Member

@BlueSlug BlueSlug left a comment

Choose a reason for hiding this comment

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

Looking pretty good, though there are still many changes that need to be made before I'm comfortable giving my approval for merging.

Beyond what's noted in individual files:

  • The linter is failing. Please be sure to lint the repo before every commit. I often forget this, so I use a git pre-commit hook that does it automatically. You can use this one if you like: https://raw.githubusercontent.com/fluid-project/sjrk-story-telling/master/pre-commit
  • There are still styling & spacing inconsistencies between this site and the live site. Please look into this, since it should be a direct 1:1 port of the old site
  • I recommend checking out GitHub's Markdown guide for some tips on Markdown formatting
  • The two layout files default.html and posts.html are 99% identical. Consider ways to remove this duplication
  • Please make sure all JavaScript files that are original to this repo have the copyright notice in them. The one in Gruntfile.js is perfect except for the dates (the date should only be in AUTHORS.md)
  • Is .nojekyll necessary? I think it can safely be removed
  • analytics.js is empty. Please either remove it or add code to it
  • There are many references to "FLOE" that should be "Fluid". I also often get tripped up doing copy-paste from other code, so it's important to be wary of this sort of thing :)

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
src/_includes/partials/header.html Outdated Show resolved Hide resolved
src/_includes/partials/header.html Outdated Show resolved Hide resolved
src/posts/2009-08-12-Fluid_Infusion_1_1_1_released.md Outdated Show resolved Hide resolved
src/posts/2009-08-12-Fluid_Infusion_1_1_1_released.md Outdated Show resolved Hide resolved
src/posts/2009-08-12-Fluid_Infusion_1_1_1_released.md Outdated Show resolved Hide resolved
@sachin10101998
Copy link
Contributor Author

sachin10101998 commented Jun 6, 2020

@BlueSlug
I have addressed all the review comments and made the changes as required.

  1. nojekyll file has been removed
  2. Added code to analytics.js as prescribed by Gtirloni
  3. Made all conversions from floe to fluid.
  4. Added the right copyright template to all original JS files.
  5. default.html and post.html are almost identical but when I try to merge them, infusion uio breaks for some reason unknown and it doesn't work and console shows errors. I have tried all methods to combine them but until and unless they are in different formats with different names, infusion breaks. Hence I kept them separate.
  6. The inconsistency in the live site and migrated site is due to the fact that I changed the UIO preferences in order to keep it in sync with floe project. It made no sense to have different stylings for the same thing and a consistent style for the uio button provided with consistency. Sue to different shapes of the button, the site shifts a bit low compared to the original site, but the migrated site is in accordance with floe project.
  7. Markdown inconsistencies are being caused due to usage of HTML tags in the MD files. These HTML tags come from the original news articles. i really don't know how to go around this. We can either remove the posts folder from markdown linting or we'll have to rewrite those posts in accordance with markdown supported tags.

@sachin10101998
Copy link
Contributor Author

@BlueSlug I found a workaround to deal with styling while keeping all the posts in markdown. I have made the required changes in both floe and fluid. Please check.

Copy link
Member

@BlueSlug BlueSlug left a comment

Choose a reason for hiding this comment

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

It's looking really great! The Markdown files are nearly perfect now, just a handful of formatting issues but nothing too complicated :)

<!doctype html>
<html lang="en" dir="ltr">
<head>
<title>{% if title %} {{ title }} {% else %} {{ site.title }} {% endif %}</title>
Copy link
Member

Choose a reason for hiding this comment

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

Please add | fluid at the end of the title, since every page on the current site has this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<!doctype html>
<html lang="en" dir="ltr">
<head>
<title>{% if title %} {{ title }} {% else %} {{ site.title }} {% endif %}</title>
Copy link
Member

Choose a reason for hiding this comment

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

Please add | fluid at the end of the title, since every page on the current site has this as well

(E.g. the News page on the current site has the title News Archive | fluid, whereas the current version of the 11ty site has News)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<!doctype html>
<html lang="en" dir="ltr">
<head>
<title>{% if title %} {{ title }} {% else %} {{ site.title }} {% endif %}</title>
Copy link
Member

Choose a reason for hiding this comment

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

Please add | fluid at the end of the title, since every page on the current site has this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

src/index.html Outdated
@@ -0,0 +1,45 @@
---
layout: layouts/default
title: The Open Source Software Community - Fluid Project
Copy link
Member

Choose a reason for hiding this comment

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

Please remove - Fluid Project from the end of this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

---
layout: layouts/default
permalink: infusion.html
title: Infusion | fluid
Copy link
Member

Choose a reason for hiding this comment

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

Please remove | fluid from the title of this page (since it should be added to the layout template)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

author: Release Manager
---
**The Fluid Project is pleased to announce the release of Fluid Infusionv0.3, the latest release of the**
**Fluid component library and User Experience (UX) Toolkit.
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the ** at the beginning of this line and add it at the end, the whole paragraph is meant to be bold.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


### What&#39;s New in This Release?</h3>

- The Reorderer, a JavaScript library for sorting DOM elements ###
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the ### from the end of this line and the four lines after it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Fluid is still in early development, and this release has a number of known issues. API changes will be coming
in the future, so for now, we encourage you to use 0.1 for experimentation and prototyping. Check the
README.txt file in the download package for a list of known issues.
Thanks to everyone in the community for their hard work on this release!
Copy link
Member

Choose a reason for hiding this comment

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

Please add an extra line break before this line, this is meant to be two paragraphs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

The Fluid Project is pleased to announce the first release of the Fluid component library and
User Experience (UX) Toolkit.

### What&#39;s New in This Release?</h3>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all instances of </h3> from this file. Search-and-replace can often lead to leftover bits of markup like this, it's important to check afterwards :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Gruntfile.js Outdated
sources: {
json: ["package.json", ".eslintrc.json","./src/_data/*.json"],
js: ["./src/assets/js/*.js","./src/transforms/*.js","./src/filters/*.js","./src/assets/js/*.js","./src/filters/*.js","./src/utils/*.js",".eleventy.js","Gruntfile.js"],
md: ["./src/posts/*.md"]
Copy link
Member

Choose a reason for hiding this comment

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

Please include all other Markdown files in the project in the md linter (README.md, AUTHORS.md, CONTRIBUTING.md, etc.), except for the news post template and anything in the lib directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added README.md, AUTHORS.md, CONTRIBUTING.md, etc.. The lib folder is not added because it's being copied from infusion and we lint only third party files. We discussed this earlier and came to a conclusion that this folder does not need to be linted. Therefore, I didn't add it.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks!

Other files that I would suggest adding to this list, other than the three you just mentioned, would be:

  • ./_data/README.md
  • ./.github/ISSUE_TEMPLATE/bug_report.md
  • ./.github/ISSUE_TEMPLATE/feature_request.md

and maybe ./src/assets/stylesheets/foundation/README.md since it's not copied in as a 3rd-party file, but I'm less concerned about that one.

Copy link
Member

@BlueSlug BlueSlug left a comment

Choose a reason for hiding this comment

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

It's looking nearly ready to merge! Excellent work @sachin10101998 👍

The only things I would like to see before giving my approval for merging are what we discussed in our video call yesterday:

  • output news files should be placed in the news dir and files should have the same filenames as the current site (so no outside links to the Fluid Project site get broken)
  • implement the slug field in the front matter and use that for the output filename if it's present, falling back to title if not
  • as @jhung suggested in this comment, it would be better if the startup script was called start instead of eleventyport, since the name is unexpected compared to our other projects, and that it would include a new clean script before building

@sachin10101998
Copy link
Contributor Author

sachin10101998 commented Aug 12, 2020

@BlueSlug I have made all the required changes. Also, the timezone difference bug won't happen now as I removed that filter. it was not needed.

Copy link
Member

@BlueSlug BlueSlug left a comment

Choose a reason for hiding this comment

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

I feel this is ready to be merged in. @jhung has agreed to provide a final review, but if any other fluid-project committers would like to, it would be welcome :)

I have also filed a small PR against @sachin10101998's branch which tidies up CONTRIBUTING.md a little bit, but I don't think this is critical to solve ahead of merging.

@gtirloni: is there anything that should be added for deployment to work properly once this is merged in? There are already the files you added to build the image and push it to Docker Hub, of course.

@gtirloni
Copy link
Contributor

@gtirloni: is there anything that should be added for deployment to work properly once this is merged in? There are already the files you added to build the image and push it to Docker Hub, of course.

I'll need to configure the servers to properly deploy this and the changes aren't very simple. I'd prefer to work on this in a separate PR, if that's okay.

Clarified instructions for running project locally
@BlueSlug
Copy link
Member

@gtirloni: I have no objection to that being covered in a separate PR as long as merging this into fluid-project/master won't impact the deployed site :)

@gtirloni
Copy link
Contributor

@BlueSlug excellent point! I've disabled the existing Jenkins jobs for the fluid and floe websites so it won't deploy anything. Thanks!

README.md Outdated

1. Get the required node modules: `npm install`
2. Run docpad from the fluid-website directory `npm run docpad`.
3. Open `http://localhost:9778/` to see the website.
2. Run [11ty](http://11ty.dev) from the fluid-website directory `npm run eleventyport`.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be updated with npm run start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

CONTRIBUTING.md Outdated
`layout: layouts/post`
`title: Title of News Article`
`date: 'YYYY-MM-DD'`
`tags: post`
Copy link
Member

Choose a reason for hiding this comment

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

If tags are not being used, then it should be removed from the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

CONTRIBUTING.md Outdated
Comment on lines 39 to 40
`permalink: YYYY-MM-DD-newsArticleName.html`
`filename: Short URL for news. May contain Capital Items but no spaces`
Copy link
Member

Choose a reason for hiding this comment

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

It's confusing to have both permalink and filename. The permalink should be removed since it is automatically generated from the {date}-{title}.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

"lint": "grunt lint",
"clean": "rimraf dist",
"cleandependencies": "rimraf node_modules",
"cleanall": "rimraf node_modules && rimraf dist"
Copy link
Member

Choose a reason for hiding this comment

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

I can not seem to successfully run cleanall. To reproduce:

  1. Run npm i then npm run build
  2. Run npm run cleanall
    The following would appear in the error log:
13 verbose stack Error: fluidproject.org@1.0.0 cleanall: `rimraf node_modules && rimraf dist`
13 verbose stack Exit status 1
13 verbose stack     at EventEmitter.<anonymous> (C:\Users\jhung\AppData\Roaming\npm\node_modules\npm\node_modules\npm-lifecycle\index.js:332:16)
13 verbose stack     at EventEmitter.emit (events.js:223:5)
13 verbose stack     at ChildProcess.<anonymous> (C:\Users\jhung\AppData\Roaming\npm\node_modules\npm\node_modules\npm-lifecycle\lib\spawn.js:55:14)
13 verbose stack     at ChildProcess.emit (events.js:223:5)
13 verbose stack     at maybeClose (internal/child_process.js:1021:16)
13 verbose stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recreated the scenario and I do not see this error.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for testing. I've filed an issue (#61 ) so it can be investigated later.

Copy link
Member

@jhung jhung left a comment

Choose a reason for hiding this comment

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

Looks good!

@jhung jhung merged commit 772f263 into fluid-project:master Aug 24, 2020
@jhung
Copy link
Member

jhung commented Aug 24, 2020

@gtirloni This PR is now ready for deploy testing. Let me know what the next steps are.

@gtirloni
Copy link
Contributor

@gtirloni This PR is now ready for deploy testing. Let me know what the next steps are.

Thanks! I'll start working on that tomorrow morning.

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.

None yet

6 participants