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

184994821 reformat scholarships page #21

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

Conversation

EthanqX
Copy link

@EthanqX EthanqX commented Apr 20, 2023

Replaced original styling with simplistic Bootstrap to keep style consistent with rest of app

Copy link

@simonjov simonjov 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 overall. Please just remove the DS_Stores from the commits and address my other comments and we should be good to merge after that!

.DS_Store Outdated

Choose a reason for hiding this comment

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

This file should not be added to the commit. You should add it to the .gitignore.

Choose a reason for hiding this comment

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

This file should not be added to the commit. You should add it to the .gitignore.

app/.DS_Store Outdated

Choose a reason for hiding this comment

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

This file should not be added to the commit. You should add it to the .gitignore.

Choose a reason for hiding this comment

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

Why are these tests commented out? Please either add a comment explaining this and maybe even a TODO to give guidelines for what future developers can do to fix them or just remove the tests altogether.

Copy link
Author

Choose a reason for hiding this comment

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

Do I have to close this PR and re-open a new one after I fixed the issues?

@EthanqX
Copy link
Author

EthanqX commented Apr 26, 2023

@simonjov Hey Simon, I just added the DS_Stores to .gitignore, and removed the outdated test

.gitignore Outdated
@@ -40,3 +40,8 @@

# Ignore application configuration
/config/application.yml

# Ignore all the DS_Stores
/.DS_Store

Choose a reason for hiding this comment

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

You can do simply .DS_Store without including a / or any directories.

You will likely want to remove all the existing ones. find . -name .DS_Store -delete should do it

Copy link
Author

Choose a reason for hiding this comment

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

Fixed it!

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.

5 participants