-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat(website): style View Campaign page #100
Conversation
Test failures look unrelated. (FYI @dinagraves I think there's a bad filename somewhere.) |
It's failing b/c the trigger is looking for the test config in the main branch but it hasn't been merged yet (#94). It will also fail anyway because you didn't write any tests :). |
Now it fails because you have no tests :)
|
</div> | ||
|
||
<!-- Campaign info (top right quadrant) --> | ||
<div class="emblem-campaign-details mdc-layout-grid__cell mdc-layout-grid__cell--span-6"> |
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.
Discussion: We've generally been using HTML4 markup in this project, I think we should consider using more semantic HTML5 markup. For example, using main, section, aside, header, and footer in lieu of many divs.
Raising this because I think we're approaching a stage where it goes from "revising to meet a new standard" to "planning a refactoring strategy"
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.
I agree, but I think this is best addressed as a codebase-wide refactor (since other already-styled pages do the same thing).
I filed #106 to track this effort.
@dinagraves - @engelke and I will have to discuss a larger frontend/website testing plan going forward. That's probably not going to be addressed for a few weeks, though. (Ideally, I'd like to get the website fully styled first.) |
Sure - you have a few options then. Either someone who has override power can merge the PR, or you can write a simple "test" that just does |
* feat(website): style View Campaign Details page * Address comments * Placate pytest Co-authored-by: Dina Graves Portman <dinagraves@google.com>
Fixes #64