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

Website update #3059

Merged

Conversation

peter-michalski
Copy link
Collaborator

Updates for #3039

@smiths smiths marked this pull request as ready for review September 29, 2022 19:40
@smiths
Copy link
Collaborator

smiths commented Sep 29, 2022

@peter-michalski, what is the best way to review these changes. I've clicked around through the code, but it would be nice to see the web-page itself, rather than the code that generates it. Is there a way to generate the web-page from this branch to look at it?

@peter-michalski
Copy link
Collaborator Author

peter-michalski commented Sep 29, 2022

Is there a way to generate the web-page from this branch to look at it?

@smiths, you will need to clone my repo, go into the branch and make website. The HTML file will be in Drasil/code/Website/HTML/

@smiths
Copy link
Collaborator

smiths commented Sep 30, 2022

Thanks @peter-michalski. Building the webpage on from your fork was very helpful for reviewing the finished product. By the way, why are you using a fork instead of a branch? Is it something to do with building the webpage?

The new webpage looks good to me. I like the reorganization and the links to more detailed information.

I didn't do a deep dive, but one thing I couldn't find was instructions to build the website, like the instructions you gave me. 😄 I think we should include these for future website maintainers.

Copy link
Collaborator

@smiths smiths left a comment

Choose a reason for hiding this comment

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

I approve of the new webpage, but I'll leave it to @JacquesCarette to determine if the webpage is ready to be merged.

@peter-michalski
Copy link
Collaborator Author

By the way, why are you using a fork instead of a branch? Is it something to do with building the webpage?

No, just out of habit I work in forked repos when I am not an owner or frequent developer. I started working with Drasil in the same mindset, but now that you mention it, it might be a good idea to go ahead and branch the main repo when working on future issues.

@JacquesCarette
Copy link
Owner

Yes, we definitely want you to become a "frequent developer" of Drasil! (Review of this incoming)

Copy link
Owner

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

As far as improving the website, this achieves that aim, in a way that's completely consistent with what is already here - thus the approval.

A deeper look at the code reveals all sorts of opportunities for further improvement. But it doesn't make sense to hold up these improvements for that, but make them another round.

@JacquesCarette JacquesCarette merged commit 1ab7a8c into JacquesCarette:master Oct 4, 2022
@balacij
Copy link
Collaborator

balacij commented Oct 10, 2022

The new website looks great! 😄

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.

4 participants