-
-
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
Lots of update to standardize events page #1117
Conversation
- 72px margin between between each Meetup category - 16px margin between subheadings and paragraphs - 80px margin between last paragraph and Meetup button
- Add 72px margin between heading and first paragraph (same as in the Meetup column card)
- Onboarding link leads to wrong url- should be to Getting Started page (https://www.hackforla.org/getting-started)
- Make time zone text bold, should be font-weight: 700 - In .userTimeZone class
- Make all of the days of week ALL CAPS (MONDAY, TUESDAY, etc. - in the html), bold, and 18px - should be- font-size: 1.125rem (18px) font-weight: 700 + in HTML: all-caps titles - In .day-header-1 class - Add 30px margin above the days of week and 10px margin belo margin: 30px 0 10px 0
- Remove the padding that pushes the meeting times to the right- Add: padding: 0 - Add to .day-header-1 class
- created new yml files to loop over events in left-col-container.html - Adjust banner font size, weight, and margin - should be - font-size: 1rem (16px) font-weight: 700 margin: auto 29%
- Decrease the padding of the page cards in their collapsed form- should be- padding: 5px 15px - Adjust the border radius- should be - border-radius: 20px
Hey @jbubar @drubgrubby . Please take a moment to review this pull request
Please await @daniellex0 styling edits,
|
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.
@akibrhast - Really impressive work. You really took a lot of different issues on the events page, saw a different way to look at the issue, and then took it apart and put it back together in a new way that solves a lot of the issues.
I am curious that you deconstructed the page and created all of these files to include, but left the javaScript on the .html page rather than moving it to a .js file and including it. Is there a reason that you left it here?
That said, it's a different approach to structuring the site and building the pages, and even though I like it personally, I think it needs a discussion about if we want to go in this direction.
@cnk @ExperimentsInHonesty - When you have a sec, can you weigh in on Akib's restructuring of the way the events page is built? It seems to work, but it's also a change from how we have been doing things, so a discussion seems warranted.
I'm not looking at any of the design questions. I will leave that for Danielle.
- Moved content in the previously second script tag into righ-col-content.html. Because everything the code within that script tag is doing is very tighty tied to everything within the right-col-content.html, i.e the scheduler.
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.
Ok @akibrhast sorry about the delay- thank you for making these changes! Some styling edits before this can go live (I think some of these issues are from a previous update to the page, but I appreciate if you can fix these to make the page look good for now before the other issues are done!):
- Can you please actually remove the
content-section
class? It's causing the yellow banner to have 16px margin on both sides, which makes it look especially weird in mobile-
I'll make this into a separate issue so that someone can figure it out after, because there seem to be some potential solutions (or we might just have to forego this standard class on this page.. not sure).
- Please align the images with the text in both left and right column cards-
So instead of this-
it should look closer to this-
I believe this can be done by putting the text and image in the same div, and their new class should have the following properties (only for above mobile size):
display: flex;
align-items: center;
-
In
.notification-banner p
class please remove theline-height
property (it should go to default line height) -
Please make these highlighted changes to the collapsed column cards:
- Please remove these lines:
- And this is my bad, I made this edit to the text on Figma but I'm realizing it actually looks too big, so please change the font size here back to 1rem (but keep the html text in all caps) and remove the line height, as seen here:
That should be good for now, I'll make other edits on the other pages. Please just make sure everything (including my edits here) look fine in mobile before it goes live :)
…y (it should go to default line height)
- Can you please actually remove the content-section class? It's causing the yellow banner to have 16px margin on both sides, which makes it look especially weird in mobile-
@daniellex0 Made all the changes as request! |
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.
Thank you for making all of the changes @akibrhast ! Ok, truly, last changes I promise (any others will go in the other issues). Sorry, there are just some margin issues that I can't let go live on the site!
Just make these changes to these 4 classes please and you'll be done:
margin-bottom instead of margin-top
Thank you!!
@daniellex0 best ever front end issue i had to deal with.... you tell me exactly what and where needs to be changed ... I go in an change it! 👯♂️ 👯♂️ |
Styling approved 👍 |
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.
Really really cool stuff. I always learn from reading your code..
A few things that I noticed that were not a part of this issue...
maybe we should have these be scrollable?
and this does not look so good in mobile view
But I know that teddy already created an issue for that.
I will finish my review later.. @akibrhast just informed me that our live wins page is down
@jbubar I have already created a separate issue that address the change you requested . There is already a LOOOOT of changes on this one pull request(Not proud of it and I am sorry to whoever is having to review this). Should we add more into it? The our-locations-content.html is a repeatable pattern that needs to be in a for loop somehow. That is why I did not address any of the changes requested in figma for that particular section. I have spoken about this to @daniellex0 and she has agreed on separating it into a separate issue. The same goes for the crappy mobile display, which @TeddyTelanoff made an issue for.
I would be very averse to doing this I fear that if I make the change now, the bigger problem of having three hard coded divs, which themselves has 6 to 8 hard coded divs will be kind of forgotten! thoughts? opinions? |
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.
Thanks Akib for clarifying! and thanks for creating all of those issues!
Everything looks great! I like the new structure!
Here is a similar restructure with the wins page #1132 |
A new issue should be created before merging this, that should be put into icebox and made into a discussion regarding the directory structure going forward at hack4la |
This pull request is in relation to #1041 and it
closes #1109
closes #1114
closes #1112
closes #1113
closes #1111
closes #1116
😄 😄