-
-
Notifications
You must be signed in to change notification settings - Fork 785
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
Guides Pages Redesign #3161
Guides Pages Redesign #3161
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Converting this PR to draft as it is not ready to be reviewed yet. Also, planning to create a new feature branch so that the conditions in special notes above could be satisfied. |
Availability: 3 hours |
Based on further discussion during the team meetings, this PR is ready for review. We are just going to merge this to gh-pages branch. Factors considered for this decision: it takes a lot of time to manage feature branches. Also, since the link for this page isn't shared on our hfla website yet, so the page does not have a lot of visibility and it should be fine even though #2978 isn't done. |
Avail: 2hrs |
Ongoing Review (Revised ETA)
Else:
|
Thanks for reviewing my PR.
Else:
Thank you again for reviewing my PR. Please feel free to reach out to me if you have further questions! |
Sorry for the delay, its been a busy week - |
Revised ETA: Thursday EOD |
Review ETA --> Hi @alyssabenipayo , sorry for the late update. It is a pretty large issue, @tamara-snyder and I will get together at some point to review together and also work individually to review it. Hopefully have it reviewed by the end of this week -> Sunday (July 25th, 22) EOD. |
Hi @usaboo, all good! Thanks for letting me know! Please feel free to let me know if you have any questions. I'm still available to make any changes if needed. 👍 |
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 for the work on the issue! There's a few things I spotted that were seemed kind of off with the PR that could be addressed. I've listed them down below and pointed out the spots where changes might be needed in the code itself if they're possible to resolve.
-
Clicking on the links on the right side seem to redirect you to the line immediately below the label as opposed to the line corresponding with the link text itself. I'm not too sure if this is intended behavior or not.
Example: Clicking on What is Slack? gets you slightly below the header for What is Slack? -
From the original Figma, it looks the sections: Who uses reminders and Type of Reminders (line 44 and line 49 in _guide-pages/how-to-set-reminders-in-slack.md should be using the the same header CSS as What is Slack? and When to use Reminders?.
This should just be a change of these first lines to the second set:
## Who uses reminders
## Type of Reminders
# Who Uses Reminders
# Type of Reminders
-
In When to use Reminders (line 51 of _guide-pages/how-to-set-reminders-in-slack.md) , there's some strange text that goes: **allows you to set up an automatic reminder with a tailored message **
It doesn't look like this text is meant to be bolded according to the Figma page. -
There's a numbering issue on the page under option 1 (starting below line 73 to line 99 of _guide-pages/how-to-set-reminders-in-slack.md), as the steps all start with 1. instead of being properly numbered from 1-4.
-
A similar issue is seen in Option 2: by Direct Entry (starting below line 114 to line 123 of _guide-pages/how-to-set-reminders-in-slack.md) where the numbering resets on the step starting with "To Whom"
-
Also on the second step (line 119 to 121 of _guide-pages/how-to-set-reminders-in-slack.md) it looks like there should've been subpoints or some other special formatting for the reminder recipients, "specific person" and "entire channel" since they both appear on the same line instead of separated. The differences are shown below between what's on the current page and the figma design.
@Wny-Duong Thanks so much for your feedback! I'll be picking up this PR with proper responses either tonight or next Monday as I'll be on vacation for the rest of the week. ETA: Friday, 8/19 the latest |
@Wny-Duong Thank you so much again! I greatly appreciate your time in reviewing this large PR and the details in your comments. 😄 Please see below for my responses.
Great catch! (I'm going to circle back to this later and I investigate. Will update this as soon as it's resolved.)
No changes were applied. The font sizes for See Here for Font Details
Done ✅ That was an oversight on my part when I created the Figma design. The text was supposed to be bolded to match the original document in our Google Docs. I've applied the changes by fixing the bolded text in the
Done ✅ Added a line break to add a space between the steps and the images, specifically for steps 1, 2, and 4, so that Jekyll can recognize that the steps should be properly numbered from 1-4.
Done ✅ Applied the changes similar to Option 1.
Done ✅ I've also updated the font color to the Figma since apparently I didn't add the color the "specific person" and "entire channel" as it appeared on the Google Docs. Applied changes can be seen in the previous comment. Side note/Commentary: I think ideally (at the time) we wanted to avoid applying CSS (and HTML) to the MD files, but applying CSS was how I could replicate what we had in our design...at least until we find out how to add that to our Automated MD Converter. Other than that, I'm open to suggestions on how to best apply the special formatting to this section. 🙌 Side-Side Note: Not done updating this PR. Committing changes in the meantime till I come back tomorrow or Friday. Will update you all once it's resolved. Thanks! |
Update 8/18/22: Issue resolved. These links helped. The only thing is that I need to create a new PR...@Wny-Duong |
Closed PR. Recreated here - #3476 |
Fixes #1630
What changes did you make and why did you make them?
This pull request contains files to the Guide Pages Redesign.
The following files have been added to the respective folders:
_guide-pages\how-to-set-reminders-in-slack.md
- New Guide Page called "How to Set Reminders in Slack"_includes\toc.html
- Generates section and subsection titles for the sticky navigation of the Guide Pages._layouts\guides.html
- Layout used for the Guides' MD files._sass\components\_guides.scss
- Styling for the redesigned Guide Pages.assets\images\guides\how-to-set-reminders-in-slack
- Contains all of the images included in "How to Set Reminders in Slack"assets\images\guides\guide-page-hero.svg
- New hero image for the redesigned Guide Pages.assets\js\guidepages.js
- JS for Guide Pages that enables the functionality of the sticky navigation bar and the Return to the Top Button.pages\guides.html
The following file has been modified:
_sass\main.scss
- Added@import 'components/guides.scss';
to import styling for the redesigned Guide Pages.Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Current Guide Page Design
Visuals after changes are applied
Redesigned Guide Page Design
Note: Please excuse the odd navbar placements. I used an extension to snapshot the entire webpage and the navbar followed the extension as it scanned the page.
Here is also the Figma Link to the Guide Pages Redesign as reference.
Link to View Locally: http://localhost:4000/guide-pages/how-to-set-reminders-in-slack.html
Special Notes
The following issues need to be completed before the Redesigned Guide Page is ready for public view:#2978 - Create Guides Markdown Converter (Google Apps Script)
Update 6/4/22:
After a discussion with our team, PR will be merged into gh-pages after review.