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

Dynamic User Interface: removing smooth transition requirements and mobile menu section #27775

Conversation

Sokolan
Copy link
Contributor

@Sokolan Sokolan commented Apr 10, 2024

Because

This PR

  • removing the mention of smooth transitions in the "image slider" section.
  • removing the entire mobile menu section.
  • removing knowledge check, as it only had questions regarding the mobile menus section.
  • removing the block-quote in the Introduction that links to the video on animations
  • fixing a typo clickable, instead of click-able

Issue

Closes #27247

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@github-actions github-actions bot added the Content: JavaScript Involves the JavaScript course label Apr 10, 2024
@MaoShizhong MaoShizhong self-requested a review April 10, 2024 13:35
@MaoShizhong MaoShizhong self-assigned this Apr 10, 2024
Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sokolan Thanks for doing this. There are a couple of things that would need doing that seem to be caused by your fork not being up to date with the upstream repo before you made your branch.

  • Therefore, you're missing the markdownlint files which will allow you to do lint checks locally, as well as our makrdownlint action to run on your PR.
  • You were also therefore editing an out of date version of the dynamic UI lessons md file. Fortunately, it looks like the only difference between the one you were working on and the current one is the additional of an Additional resources section.

In the interest of simplicity, it may be easier at this point to close this PR, checkout to main in your local fork clone, sync that with upstream main (git fetch upstream main followed by git merge upstream/main), then create a new branch off that up-to-date main.

Then that branch will be up-to-date as well and you can redo your changes. The lint files will be present so lint checks can also run.

About the changes made, we can't get rid of the knowledge check section, as that's a mandatory section of our lesson layouts (and when lint checks happen, it expects this section).

Before you do anything further (you would definitely still want to update your fork though), @thatblindgeye what do you think about having this lesson converted to a project? It would make far more sense having the drop-down and image slider parts as a formal assignment, and it will allow us to drop the lesson overview/knowledge check/additional resources sections which are a little awkward given the contents of this lesson.
Doing so would of course also require making changes to the appropriate parts in the website repo.

@MaoShizhong
Copy link
Contributor

@Sokolan Thanks for your patience with this. Just been discussing with @thatblindgeye about the most appropriate way forward with this.

We'll keep the lesson as a lesson for now, and won't convert to a project. We will need to add back in the knowledge check section though, as well as the additional resources section that was missing due to your old fork.

Feel free to close this PR and start a fresh one with an up-to-date fork and branch if you feel it simpler, or you can get this branch up-to-date - whatever you prefer. As long as we can work with a PR that is up-to-date with the linting stuff and current lesson contents.

We were thinking to update the introductory paragraph for the ### Drop-down menus section to something a little more explanatory, for which we can then craft new knowledge check questions that'd actually be relevant. Perhaps something like:

A dropdown is something you've most likely seen on various websites you've visited or apps you've used. Imagine any time you've clicked a button (typically one that contained an icon of 3 horizontal or vertical dots) and a list of items suddenly appeared (you could even say these items... dropped down from the button).

Dropdowns are typically comprised of two parts: a button that toggles the dropdown content's visibility, and the dropdown contents itself.

The dropdown toggle should typically only trigger the visibility of the dropdown content on click, while the dropdown contents should typically only contain items that will trigger an action upon clicking them. Actions can include things like "Edit", "Copy", or "Delete", or linking you to another part of the site, such as in a navbar.

Now that you have an understanding of what a dropdown is, let's make one!

Then having knowledge check questions such as

- [What two main parts does a dropdown menu comprise of?](#drop-down-menus)
- [When might you want to use dropdown menus in a website?](#drop-down-menus)

Does not have to be that exact verbiage above, that's just a suggestion so feel most free to tweak it if and as you see fit. We can always refine in a review as necessary. But something among those lines to replace the "You know what we're talking about here..." paragraph.

And while you're at it, would you also mind fixing up the linting errors in the lesson too since you now have the tools for that with your updated fork? The errors should be mostly self explanatory like using lazy numbering for lists, or changing _ emphasis markers to * etc.

Let us know if you have any issues.

@Sokolan Sokolan closed this Apr 19, 2024
@Sokolan
Copy link
Contributor Author

Sokolan commented Apr 19, 2024

@MaoShizhong Thanks for the update, and the instructions.

I got a brief question that I couldn't find an answer to in the LAYOUT_STYLE_GUIDE.md, if there's no assignments, what should be the default text?
The Additional resources have a default one (and by the linter so does Knowledge check), but the linter doesn't show any error if I leave Assignments empty.

@MaoShizhong
Copy link
Contributor

@Sokolan I think we completely missed the fact that the lesson is also missing an ### Assignment section altogether, and needs it (where there isn't a default content rule - only that its sole contents is a <div class="lesson-content__panel" markdown="1"></div> Where the assignment stuff goes in that div.

If you give me a bit, I'll think of the best way to approach structuring the lesson so we have the same contents but following the appropriate lesson structure. Right now, just wrapping the dropdown and image carousel sections in an assignment div looks odd given the amount of non-assignment text (the explanation stuff).

@MaoShizhong
Copy link
Contributor

@Sokolan So to get everything structured better while also appeasing the linter, we can do the following.

After the Lesson overview section, we have the following structure:

### Dropdown menus

< Put the dropdown menu explanation paragraphs here - get rid of the "let's make one" sentence >

### Image carousels

You've likely seen many image carousels across various websites, such as on shop home pages. They're great for advertising or showcasing things, and can actually be made using things you've already encountered! They are also very highly customizable - you can make them autoscroll, allow users to manually cycle between slides or skip to certain slides etc.

Typically, they consist of a div that acts as the "picture frame", where behind that div, there is another much wider div containing the carousel's images. This strip of images can then move behind the picture frame, showing a different image depending on what part of the strip is visible. Any additional controls or features can then be placed on top of the entire thing.

### Assignment

<div class="lesson-content__panel" markdown="1">

#### Build a dropdown menu

< Put dropdown menu ordered list instructions here >

#### Build an image carousel

< Put image carousel ordered list instructions here >

</div>

Then the knowledge check and additional resources sections can go after the assignment div closes.

A few points:

  • The Image carousel section text above is again, only a suggestion. Feel most free to amend it as you see fit, and we can always discuss specifics in review.
  • Could you also add to step 3 of the Build an image carousel instructions that the person does not need to animate this? Just to make it extra clear it's not a requirement.

I know a lot more has developed from the original "remove transitions/animation" issue, but this will kill a few birds with one stone while we're at it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: JavaScript Involves the JavaScript course
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic User Interface Interactions: Removing the requirement for smooth transition
2 participants