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

Signin page styling #87

Merged
merged 4 commits into from
Apr 25, 2019
Merged

Signin page styling #87

merged 4 commits into from
Apr 25, 2019

Conversation

TangoYankee
Copy link
Member

@TangoYankee TangoYankee commented Apr 14, 2019

Checklist

  • Add description
  • Reference open issue pull request addresses
  • Pass linting check
    • complete on the local machine with make local.shell make quality
  • Pass functional tests
    • complete on the local machine with make local.shell make test
  • Request code review
    • Please allow 36 hours from opening a pull request before merging a pull request- even if it has already recieved an approving review.
  • Address comments on code and resolve requested changes
  • Merge own code

Description

Issue: #71

Responsive sizing of signin form, text to give users context, password toggle, email to request account.

@theecrit
Copy link
Collaborator

See my comment on #71, please and thanks!

Copy link
Collaborator

@nbilenko nbilenko left a comment

Choose a reason for hiding this comment

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

For the h1/button/general element styling, does that need to be localized? It seems like the style can be general across the site.

@TangoYankee
Copy link
Member Author

For the h1/button/general element styling, does that need to be localized? It seems like the style can be general across the site.

@theecrit This is a question of implementing your design. Do you want the links in the navigation bar to have their style extended to all buttons in the site?

@theecrit
Copy link
Collaborator

theecrit commented Apr 19, 2019

Hi all, sorry for the delay. I'm working on figuring out how to get the latest build on my machine so I can see where we're at, but to try and answer the questions above:

For the h1/button/general element styling, does that need to be localized? It seems like the style can be general across the site.

Yes, we probably want it generalized into a site-wide style guide. Of course, we don't have one yet but I created a Trello card for it. For now, what specifics do you need from me to unblock this? Type styles and button styles?

Do you want the links in the navigation bar to have their style extended to all buttons in the site?

Probably not. Navigation menu should be styled differently than action buttons. Of course, nav links should be styled consistently across the site (i.e. from page to page).

@theecrit
Copy link
Collaborator

theecrit commented Apr 19, 2019

For now, how about we use the button styles in the latest incarnation of https://cfa-styleguide.herokuapp.com/cfa/styleguide (provided by Tom as a work-in-progress for brigade projects)?

@TangoYankee
Copy link
Member Author

Upon rereading Natalia's comment, I think I misinterpreted the part to which see was referring.

The question really seems to be whether to apply styles only to the current page, or extend them across the site to all elements with those tags. @nbilenko I want to default to localizing styling elements. This reduces the chance of unintentionally affecting another component.

For the CfA button styling guide, I'll implement the wireframe styling to the best of my ability. Below is the current iteration. On hover, the Signin form button turns black with white text. The right side links of the nav bar turn green. @theecrit, If you'd like to change them to the CfA styling, update the wireframes and I'll update the design.

WOEIP Signin

@theecrit
Copy link
Collaborator

A couple of things:

I’d really advise against localizing styling. Style guided and design systems exist for a reason, and trying to update every single instance of a given style should a change be needed is not tenable. Let’s come up with a better solution. Unless I’m misunderstanding the questions, as well.

Re: wireframe updates. I’d like us to practice uncoupling wireframes from redlines (pixel-specific visial design). Again, it’s about finding the path of least resistance balanced with best practice: the visual design styles should remain independent of content. The wireframes are intended to communicate content, hierarchy, relative layout, and in some cases, behavior. Visual design styles (e.g. css) need not be (and shouldn’t be) tied to wires, or we’ll add an additional layer of documentation labor that will be prohibitive to shipping. That’s why design systems like the CfA style guide exist; so developers can apply consistent styling without needing redlines. I’m happy to specify which styles should applied to specific elements in the wires, but I’m not going to be able to skin the site as we go. Of course, maybe that’s what you meant in the first place?

Thanks!
J

@TangoYankee
Copy link
Member Author

It sounds like we're identifying a few differing assumptions. They're leading to a few things we need to discuss, that are outside the scope of a Pull Request discussion.

  1. Wireframing v Redlining
  2. Style guidelines (Default Foundation or CfA)
  3. Scoping preferences for custom styling
  4. The placement of buttons in the navigation bar

* remove most custom styling. adjust header tags for accessibility. Adjust grid of signin page

* adjust password labels

* fix navigation header typo
@TangoYankee
Copy link
Member Author

See issue #92 for explanation and discussion of changes

@theecrit
Copy link
Collaborator

theecrit commented Apr 24, 2019

The main outstanding issue I'm seeing right now (if I understand the implementation correctly) is that buttons as navigation are problematic for a number of reasons. The main one is that buttons should be used as a control interaction, whereas navigation is about wayfinding via links. Best practice, as far as I understand it, is to use unordered lists and CSS styling for navigation menus. Examples:

That A11y Style Guide link also includes a mobile guideline, which stipulates a <button> element for the hamburger icon (presumably because it's a reveal control). I'd imagine it would then reveal a <ul> based menu.

Copy link
Collaborator

@r-b-g-b r-b-g-b 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 to me!

<div class="grid-x">
<div class="cell medium-6">
<p>Or request an account from the WOAQ team at
<a href="mailto:woaq@openoakland.org">woaq@openoakland.org</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The email should probably be passed from the backend as a context variable, but that can happen in the future.

@TangoYankee TangoYankee merged commit a74aaf3 into master Apr 25, 2019
@TangoYankee TangoYankee deleted the ty/signin-page-style branch May 1, 2019 03:07
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