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

Add footer #121

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

Conversation

k3KAW8Pnf7mkmdSMPHz27
Copy link
Collaborator

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 commented Aug 20, 2021

Alternatives

Research how to collect feedback in an easy and reusable fashion.
Research Google form.

Current

Adds a footer highlighting that feature requests and bug reports can be made in the GitHub repository.

Skärmavbild 2021-08-20 kl  15 46 57

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as ready for review March 16, 2022 18:11
Copy link

@plocket plocket left a comment

Choose a reason for hiding this comment

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

Looking like a great addition! I have several notes, but I think the two blocking ones are the color of the links on hover and the missing margin/padding between the content and the footer. They're not particularly breaking, so I can improve if we want to leave those for the future.

'href',
'https://github.com/codeforamerica/brigade-project-index-statusboard/issues/new?assignees=&labels=&template=feature_request.md&title='
);
});
Copy link

Choose a reason for hiding this comment

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

Do you also want to make sure the link to the repository is there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I am skipping it, I am not sure that I want the GitHub repository link at all :/
I am not sure if it adds anything, or if it is better to have it focused on requesting bug and issue reports

statusboard/src/components/PageContents/PageContents.css Outdated Show resolved Hide resolved
Comment on lines +2 to +6
width: 100%;
min-height: 5rem;
display: flex;
flex-direction: row;
align-items: center;
Copy link

Choose a reason for hiding this comment

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

These don't seem to have an effect. Maybe worth thinking about removing them.

Copy link

Choose a reason for hiding this comment

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

Also, I tend to prefer that the space an element takes up be determined by padding, etc. Using height can be hard to debug for a new person later.


p {
color: var(--cfa-white);
font-family: var(--font-family-text);
Copy link

Choose a reason for hiding this comment

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

This doesn't seem necessary and it should probably inherit from the rest of the document, so maybe worth removing.

color: var(--cfa-white);
font-family: var(--font-family-text);
margin-left: 1.0rem;
margin-right: 1.0rem;
Copy link

Choose a reason for hiding this comment

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

The links in here have a hover color that blends into the background. Changing that could be a more accessibility friendly improvement.

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

2 participants