-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
MVP events mobile page #764
MVP events mobile page #764
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.
It looks good visually to me and I agree with incremental fixes to what is there now. If Kian says code quality looks good, then it can be merged.
@ExperimentsInHonesty @drubgrubby sorry for the delays, but I will be able to give this pull request a review tomorrow. I was considering giving this a quick review before the meeting (because I can not do a review after the meeting today), but I figured doing it tomorrow and looking closer at what the code is doing would provide a better review. |
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.
@drubgrubby Good job on the changes, it looks a lot better on mobile 👍. I left some review comments on the code structure wise. I also know this got the visual confirmation, so I didn't want to make these points as request changes, but wanted to see if they were significant enough that @ExperimentsInHonesty wanted to integrate a change for them. But for me, since I was just doing the code review, responses to the review comments is all I need to give this a pass (with a response as well to the inline styling rule note I put below, since it is code related):
- On landscape for phones, the bottom head content has a sizable space between the video (screenshot below). Would we want to implement a styling rule that lowers those margins on mobile?
- On large phone on landscape, the text get's very tall and thin like you mentioned with the tablet views @drubgrubby. Would it be easy/not too much to add to extend the styling rules that control that text on mobile to fit the width of large phones on landscape? I think the toolkit styling has some good examples of custom queries for screen width.
- There are some inline styling rules in the components (that look like they were inherited rather than created by you) related to this task, but I would opt to just have that in a "clean up events page" issue so @drubgrubby can enjoy the holidays. I don't want to pile on additional things. Unless you think it is important @ExperimentsInHonesty to be done with this issue and if you don't mind @drubgrubby.
Thank you for the updates @drubgrubby 👍 unfortunately, I will not be able to get to re-reviewing today as I thought. However, tomorrow I will make it a priority |
@drubgrubby I added some commits to add back in those inline sty lings in the form of a class. It looks like the styling were adding some minor changes when in full-screen. I also changed that Other than that, I noticed on the smallest screen of iPhone 5, the display was not working (screenshot below). This is currently existing on the main site as well, so I do not think it is something from the code @drubgrubby added. The code that improves the mobile view the other screen sizes currently makes sense and look good. So I will leave it up to @ExperimentsInHonesty if she wants to merge with the iPhone 5 issue. |
Fixes #403
This makes the events mobile page look much better than it does currently. I listed it as an MVP because it is not very refined, and I haven't done a lot of cleanup on the css classes, and other things that I would normally do. I just got it to where it looks basically like the figma.
Given the relentless nature of the Jewish holidays at this time of year I don't have time to do all of the refinement and cleanup I would normally do. Given that this page is live and the current mobile display is...not very good, I'm proposing that we merge this so it is at least acceptable, and then - if it is deemed necessary - we open a new issue called something like "clean up events page mobile" and I will get it cleaned up.
If you think it's okay the way it is, then we can just get on with our lives and on to the next project.
If you think we should wait to merge it until it's perfect(ish) then I'll get to it next week.
Current events page mobile:
How it would look after this merge:
Note: I think this design is problematic as soon as you start to make the page size smaller than full screen and the text on the right starts to get narrow and push way down below the video display - this is the display that you see on a tablet. Design things are not my decision to make, but I might suggest that we make the way the mobile looks in this merge start at tablet size, so the video is in the middle of the two text blocks rather than to the left of them. Just my two cents.