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

Assets folder added. #30

Closed
wants to merge 2 commits into from
Closed

Assets folder added. #30

wants to merge 2 commits into from

Conversation

marizvi
Copy link

@marizvi marizvi commented May 25, 2021

Hello,
I have moved css/, images/, js/ to newly created folder "assets/",
and also made changes to each existing html files so that they can now link all css, images and js files from assets folder itself.

I hope your job is done for this particular issue and if not then kindly let me know.

Copy link
Contributor

@briandominick briandominick left a comment

Choose a reason for hiding this comment

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

Hi Mohammad! Thanks for the contribution. I'm wondering if there's a commit missing somehow. You seem to have added the folders to a new assets/ dir, but I still see them in the route. And I don't see the HTML changes you referenced, which would be necessary but I think can mostly be handled by changing some paths in _config.yml.

I also realized that the fonts/ dir needs to go into assets/ as well, which my original ticket did not mention.

@marizvi
Copy link
Author

marizvi commented May 25, 2021

Hello,
Sorry for the inconvenience, it was my bad that i have forgotten to delete those dir from route, i have generated a new pull request for the same, resolving the issue you have requested for..

@marizvi
Copy link
Author

marizvi commented May 25, 2021

And if you also want the fonts dir to be moved to assets folder, kindly let me know..

@briandominick
Copy link
Contributor

briandominick commented May 25, 2021

Hi Mohammad. Unfortunately, these changes would break the site, since we didn't tell Jekyll and the browser where to find the assets now.

I see you are pretty new to GitHub. So first, welcome to GitHub! Second, I'm happy to help walk you through this as much as you might want, just let me know.

It's good practice to test your contribution before you make the pull request, and again before you push each new commit to the PR branch. There are a couple of ways to test these changes on your end in this project:

The first way is to clone your fork locally (if you have not already), then make the changes locally and run the build to test. Instructions are in the Readme file.

The other way is to set up a deployment service like Netlify to build the site from your fork every time you push a commit to your fork. Then you can go see the changes on your Netlify version of the site.

@briandominick
Copy link
Contributor

Sorry I originally wrote "Unfortunately these changes have broken the site" which is ONLY true of the demo site, which you can see here: https://deploy-preview-30--asciidocsy.netlify.app

The main site and the pull request are unaffected until we merge the changes to main.

@marizvi
Copy link
Author

marizvi commented May 25, 2021 via email

@briandominick
Copy link
Contributor

I see what happened, Mohammad. It's really no problem that you're new. I teach this stuff to new folks all the time. Welcome again.

So the bundle exec jekyll serve command runs a build procedure that generates those files in _site/. Those are artifacts of the process, but the source files are all outside of that directory. And it's the source files that we want to change and commit after we see their affect on the final product: the website rendered from the generated artifacts.

This means we have to find all the places that those HTML files got their paths to the directories that re now in assets/.

I haven't actually tested this, but I think we'll need to change some paths in _config.yml. And there are likely others in _includes/head.html and maybe elsewhere. I was thinking we probably need a new variable called site.assets_path or something to universalize this, otherwise we have to prepend /assets to all the existing paths like /css and /js.

This issue may be a little big for a first ticket, especially since it involves some application design to be done right. It might be better if I help you find a different ticket to take on. Can I ask what particular interest made you arrive at this project? Do you have a background in HTML/CSS/JavaScript/etc?

@marizvi
Copy link
Author

marizvi commented May 25, 2021 via email

@briandominick
Copy link
Contributor

Again, no problem. I work alone so it's always great when someone comes along to engage at any level! And no need to call me sir either :-).

There are a few bugs that are CSS/JS only. I'm thinking #4, #13, #14, and #15. I've also just added one about the mobile theme: #33. All of these are fairly contained and won't really require Jekyll knowledge. I'd be happy if you took any of those on. I am going to close this PR for now. I probably won't get to this ticket (#28) for a while, in case you want to come back to it once you have a better feel.

@briandominick
Copy link
Contributor

Oh also #2 is open and fits the bill as well. I was expecting to add more bugs to that but people started opening distinct tickets.

@marizvi
Copy link
Author

marizvi commented May 25, 2021 via email

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.

3 participants