-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/branding #78
Feature/branding #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the link on /about
to /about/branding
throws this error:
...but refreshing or directly accessing the page works as expected. this may be related to a similar issues @Woozl discovered while working on one of the News views.
the page looks great, though. we do not need that grey text showing the path, but i believe the image should be a link to the image path, which would open the image in a new tab.
@mbwatson that's right! thank you for pointing that out. i had forgotten to update the brand portion. i'll take a look at the error as well! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this is looking really clean and professional! upon closer inspection, i see room for some code cleanup and slight re-organization. please let me know if any of my line comments are unclear.
images/branding/clearspace-logo.png
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear what length x is labeling in this image. can't be the width of that side strip because that's labeled x/2 on the other side. if it's the height, then those "x/2"s don't look accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll push this through and clean up the image later
@mbwatson, thank you for all the helpful feedback! all requested changes have been implemented except for the updated image depicting the clear-space guidelines. the plan is to merge this branch and update the logo when it is completed by the graphics team |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this all looks great. thanks for taking care of those code cleanup requests. we discussed the clearspace image--we'll fix later. i have one last nit-pick: i'd love for the user to see a little visual cue that the copy happened. however, i think there will be other opportunities for copyable text, so perhaps we improve this component when that time comes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, looks fantastic!
Issues created: |
No description provided.