Skip to content
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

Feature: Different Note Boxes in Lesson Content #3431

Closed
ChargrilledChook opened this issue Oct 15, 2022 · 17 comments · Fixed by #3838
Closed

Feature: Different Note Boxes in Lesson Content #3431

ChargrilledChook opened this issue Oct 15, 2022 · 17 comments · Fixed by #3838
Assignees
Labels
Status: In Progress This issue/PR has ongoing work being done Type: Enhancement Involves a new feature or enhancement request

Comments

@ChargrilledChook
Copy link
Member

ChargrilledChook commented Oct 15, 2022

Originally by @KevinMulhern

Why?

We are already using a standard note box which draws attention to important information https://www.theodinproject.com/lessons/foundations-installations#introduction

We could have different note boxes with different styles to give the information more context. For example
Tip - green icon/background
Warning - red icon/background
note - yellow or blue icon/background

Examples

JS Info

Warning box:
Image

Note Box
Image

@ChargrilledChook ChargrilledChook moved this from 📋 Backlog / Ideas to 🔖 Ready for Development in Main Site Oct 15, 2022
@KevinMulhern KevinMulhern moved this from 🔖 Ready for Development to 🏗 In progress in Main Site Oct 20, 2022
@cleicar
Copy link
Contributor

cleicar commented Oct 24, 2022

@KevinMulhern @ChargrilledChook @01zulfi can I work on this one next?

@KevinMulhern
Copy link
Member

I think @01zulfi is working on this one @cleicar

@01zulfi
Copy link
Member

01zulfi commented Oct 25, 2022

I haven't started any substantial work on this one yet. I'm happy to let you tackle this one @cleicar 🚀

I don't think we had any note boxes set in stone, but below is a prototype of what I had in mind:

image

@01zulfi 01zulfi assigned cleicar and unassigned 01zulfi Oct 25, 2022
@ChargrilledChook ChargrilledChook added the Type: Enhancement Involves a new feature or enhancement request label Oct 30, 2022
JuanVqz added a commit to JuanVqz/theodinproject that referenced this issue Nov 4, 2022
@JuanVqz
Copy link
Contributor

JuanVqz commented Nov 4, 2022

Where can I find an example of the following lesson-note type?
1 Tip - green icon/background
2 Warning - red icon/background

@KevinMulhern
Copy link
Member

We don't have anything in the system like those atm @JuanVqz. The couple of examples on this issue is the kind of thing we're looking for.

@01zulfi
Copy link
Member

01zulfi commented Apr 4, 2023

@JuanVqz Checking in to see if any progress has been made on this issue

@JuanVqz
Copy link
Contributor

JuanVqz commented Apr 4, 2023

@JuanVqz Checking in to see if any progress has been made on this issue

Hey, I haven't made any progress on this one

@01zulfi
Copy link
Member

01zulfi commented Apr 5, 2023

@JuanVqz Are you willing to continue this issue? If not, that's fine as well.

@JuanVqz
Copy link
Contributor

JuanVqz commented Apr 5, 2023

At this point, I cannot do it, sorry about that, and thanks for the heads up! Good day!

@KevinMulhern
Copy link
Member

Thanks for letting us know @JuanVqz

@KevinMulhern KevinMulhern moved this from 🏗 In progress to 🔖 Ready for Development in Main Site Apr 11, 2023
@luuu-xu
Copy link
Contributor

luuu-xu commented May 31, 2023

@KevinMulhern Hi, I would like to work on this issue if it is still open. I feel it is fairly easy add-on.

@KevinMulhern
Copy link
Member

Thanks @luuu-xu, it's all yours 💪
If you need any support, please let me know.

@KevinMulhern KevinMulhern added the Status: In Progress This issue/PR has ongoing work being done label Jun 1, 2023
@KevinMulhern KevinMulhern moved this from 🔖 Ready for Development to 🏗 In progress in Main Site Jun 1, 2023
@luuu-xu
Copy link
Contributor

luuu-xu commented Jun 1, 2023

Thanks @KevinMulhern, since this will be my first open source contribution, I'd like to get some help from you.
So after reading the codebase and play it around with CSS in the browser, I learnt that the from app/javascript/stylesheets/lesson-content.css, I can make colour changes to .lesson-note, that's easy.

  1. Since we are having Note, Extra, Warning, should I just add CSS classes accordingly in lesson-content.css? So when we change the .md files, by changing <div class="lesson-note" markdown="1"> to probably <div class="lesson-note warning" markdown="1">, the colour red will be applied?
  2. And also, I suppose I should just grab the colour from the design screenshots by @01zulfi?
  3. What about the corresponding icons, where can I grab them?
  4. How should I go about adding the icon as the header of every note boxes, it seems like I can't find a component I can make changes to...
  5. I also feel like if we are adding the icon as the header, it might not look as good as js.info's style where a header title follows the icon.
  6. After these note boxes are done, should I also go ahead and make changes to the curriculum markdown files so each note box has a category?

Wow that's a lot of questions, I am sorry about asking every detail of this issue, I am just eager to make this PR my great first contribution!

@KevinMulhern
Copy link
Member

No worries @luuu-xu, happy to help.

Since we are having Note, Extra, Warning, should I just add CSS classes accordingly in lesson-content.css? So when we change the .md files, by changing

to probably
, the colour red will be applied?

CSS classes sounds perfect, we're using BEM notation for our custom classes so we could add some modifier classes for each:

<div class="lesson-note lesson-note--warning" markdown="1"`

And also, I suppose I should just grab the colour from the design screenshots by @01zulfi?

We're using Tailwind color system on the site, you can find a full list of those here.


What about the corresponding icons, where can I grab them?

The easiest thing would be to use Fontawsome icons, we have it available in the app.


How should I go about adding the icon as the header of every note boxes, it seems like I can't find a component I can make changes to...

I think we'll be able to do this with a before pseudo class, Something like this should work:

.lesson-note--info > h4::before {
    font-family: "Font Awesome 5 Free";
    font-weight: 900;
    content: "\f05a";
    @apply mr-2;
  }

Markup to make this work:

<div class="lesson-note" markdown="1">
  <h4>title</h4>
</div>

I also feel like if we are adding the icon as the header, it might not look as good as js.info's style where a header title follows the icon.

Yep, I agree it looks odd with just the icon. Let's include a title after the icon like js.info are doing it.


After these note boxes are done, should I also go ahead and make changes to the curriculum markdown files so each note box has a category?

Instead of doing all three lesson notes in one PR, I'd approach it like this:

  1. Focusing on just the existing note/info box styling in the first PR - It'll be easier to manage for you and easier for us to review/release.
  2. Change the note boxes markup on the curriculum files to include titles

After that, it will be trivial to add the styling for warning and tip lesson notes since all the patterns will be established and agreed upon by everyone involved.

@luuu-xu
Copy link
Contributor

luuu-xu commented Jun 6, 2023

@KevinMulhern Hi, after some frustrating hours of setting up the environment on the M1 iMac I have, I have added the CSS variations and requested PR.

During the works, I found out the markdown files are fetched from curriculum's github url directly so I couldn't find a way efficient enough for me to add the h4 title into the note box. But I have managed to finish the work, including colour choices for dark mode with WCAG contrast standards.

Now should I go ahead and request a PR for curriculum repo where h4 title is added to the lesson note in this PR, though I'm not sure what title would best suit it. I am thinking "Make sure your OS is supported".

@KevinMulhern
Copy link
Member

Hey @luuu-xu, thanks for getting this done so quickly!

You can test out the styling locally using our markdown preview feature, you can access that by going to http://localhost:300/lessons/preview locally.

luuu-xu added a commit to luuu-xu/theodinproject that referenced this issue Jun 8, 2023
@luuu-xu
Copy link
Contributor

luuu-xu commented Jun 8, 2023

Great feature!! I have made the changes from your suggestions and also added lesson-note--tip and lesson-note--warning to tailwind safelist so now they work great. Thank you for all the suggestions!

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Main Site Jun 11, 2023
KevinMulhern pushed a commit that referenced this issue Jun 11, 2023
<!-- Thank you for taking the time to contribute to The Odin Project. In
order to get this pull request (PR) merged in a reasonable amount of
time, you must complete this entire template. -->

## Because
<!-- Summarize the purpose or reasons for this PR, e.g. what problem it
solves or what benefit it provides. -->
Different variations (note, extra, warning) of lesson note boxes draw
attention to readers for important/warning information.


## This PR
<!-- A bullet point list of one or more items describing the specific
changes. -->
- Added CSS classes to varied lesson note boxes, including the use of
corresponding icon by Fontawesome.

## Issue
<!--
If this PR closes an open issue in this repo, replace the XXXXX below
with the issue number, e.g. Closes #2013.

If this PR closes an open issue in another TOP repo, replace the #XXXXX
with the URL of the issue, e.g. Closes
https://github.com/TheOdinProject/curriculum/issues/XXXXX

If this PR does not close, but is related to another issue or PR, you
can link it as above without the 'Closes' keyword, e.g. 'Related to
#2013'.
-->
Closes #3431 

## Additional Information
<!-- Any other information about this PR, such as a link to a Discord
discussion. -->


## Pull Request Requirements
<!-- Replace the whitespace between the square brackets with an 'x',
e.g. [x]. After you create the PR, they will become checkboxes that you
can click on. -->
- [x] I have thoroughly read and understand [The Odin Project
Contributing
Guide](https://github.com/TheOdinProject/theodinproject/blob/main/CONTRIBUTING.md)
- [x] The title of this PR follows the `keyword: brief description of
change` format, using one of the following keywords:
  - `Feature` - adds new or amends existing user-facing behavior
- `Chore` - changes that have no user-facing value, refactors,
dependency bumps, etc
  - `Fix` - bug fixes
-   [x] The `Because` section summarizes the reason for this PR
- [x] The `This PR` section has a bullet point list describing the
changes in this PR
- [x] I have verified all tests and linters pass after making these
changes.
- [x] If this PR addresses an open issue, it is linked in the `Issue`
section
-   [ ] If applicable, this PR includes new or updated automated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Progress This issue/PR has ongoing work being done Type: Enhancement Involves a new feature or enhancement request
Projects
Archived in project
6 participants