-
Notifications
You must be signed in to change notification settings - Fork 530
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
Location styling #32
Location styling #32
Conversation
By "mouse scroll issue" do you mean what happens when the scroll goes over the map and switches to map zoom? That could be mostly solved by not running the map full-width, so you can scroll over whitespace. We don't want to lose the ability to zoom the map. |
Yup, exactly that. Though @shacker you'd still have the issue that if the mouse cursor is over the map your scroll will be hijacked, even if the map's smaller it doesn't really solve the issue, just makes it slightly less likely to happen. We definitely don't want to lose the functionality of users being able to click/ zoom in the map though. It's a reasonable well known problem, and the solution is to just swap out a class with JS if the user clicks to the map. |
There are some conflicts that need to be resolved. The styling on the branch is great; uncertain how the new code from master should be (or not be) incorporated. 🚀 |
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.
As I mentioned in the comment, the styling looks good.
However, the merge conflicts need to be resolved before merge, and I am uncertain as to how to meaningfully resolve as I have not focused on the front end thus far.
I'll take on the merge conflicts. I think it's probably easiest for me to address them. |
@daaray I've fixed the merge conflicts. @shacker to mention I think yesterday you committed a couple of things that conflicted with this branch direct to master. Would be good if you can review too before it gets merged. I'd suggest that we deal with any of those issues as a subsequent issue/ PR (e.g. whether to show the location index page image on the template or not) |
|
||
{% block content-header %} | ||
{% include "base/include/header.html" %} |
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.
@heymonkeyriot I had gone through and moved all of the headers into a common header include. It looks like that has been lost here. Intentional? If not, can we preserve or refactor that system? (OK if we do that after this lands).
I suspect this will also result in conflicts with the migration files in PR #47 , but at this point we should just go with whichever is completed first, then the other can manually resolve those. |
I've tested and merged this to keep the ball rolling, but it raised a few questions (I'll make separate tickets):
|
Should close #31, #24, and works towards #9
Two col location index page
[ ] If location is openLocation detail page
[ ] Highlight opening times for today() (in a timezone naive way)Fix merge conflicts
Apologies for poor branch name.