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

add inline scrims #73

Merged
merged 5 commits into from
Sep 30, 2024
Merged

add inline scrims #73

merged 5 commits into from
Sep 30, 2024

Conversation

LeoMcA
Copy link
Member

@LeoMcA LeoMcA commented Sep 25, 2024

Description

Add inline scrims throughout curriculum.

Needs to wait for the related yari PR to be merged before merging.

Related issues and pull requests

@chrisdavidmills could you check these scrims correspond to the ones you put in the doc? Feel free to update any wording/move things around as you see fit. I'll get this up on stage so you can see how it looks.

Copy link
Collaborator

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Great stuff @LeoMcA. I've checked it against my Google Doc, and all of the URLs, display titles and embed positions look to be correct. I just had a couple of minor comments.

@chrisdavidmills
Copy link
Collaborator

chrisdavidmills commented Sep 26, 2024

Perfect, I think this is ready to go. I will hold off on merging it until the associated yari work is ready.

@LeoMcA
Copy link
Member Author

LeoMcA commented Sep 26, 2024

@chrisdavidmills
Copy link
Collaborator

@LeoMcA OK, I've had a look and I think these look nice and function well. My only outstanding comments are:

  • When you click to load the scrim and it pops up full window width, you get a little message popping up at the bottom of the embed that says "OPError: Not Allowed". The embed works fine, but I wonder if this might put some users off a bit. Not sure if this is something that needs to be looked at on our side of Scrimba's side?
  • The embeds in the page are the full width of the content column. IMO, they look a bit big and in-your-face on desktop. Would it be worth considering making them a bit smaller (say 70% of column width) on desktop, and centering them vertically?
  • In my Google doc, Miruna expressed concern that one of the scrims was also linked to as a regular link. I thought about this, and wondered if we ought to remove the links that link to the same embedded scrims. Does it seem weird, or is it helpful to have regular link versions available too? Or maybe we should just remove the links in cases where you've got the embed right next to the regular link. What do you think?

@LeoMcA
Copy link
Member Author

LeoMcA commented Sep 26, 2024

When you click to load the scrim and it pops up full window width, you get a little message popping up at the bottom of the embed that says "OPError: Not Allowed". The embed works fine, but I wonder if this might put some users off a bit. Not sure if this is something that needs to be looked at on our side of Scrimba's side?

Yes, I need to raise this with Scrimba

The embeds in the page are the full width of the content column. IMO, they look a bit big and in-your-face on desktop. Would it be worth considering making them a bit smaller (say 70% of column width) on desktop, and centering them vertically?

I've made them a little smaller, will deploy the change to staging

In my Google doc, Miruna expressed concern that one of the scrims was also linked to as a regular link. I thought about this, and wondered if we ought to remove the links that link to the same embedded scrims. Does it seem weird, or is it helpful to have regular link versions available too? Or maybe we should just remove the links in cases where you've got the embed right next to the regular link. What do you think?

I'd keep the regular links, at least for now: we haven't fully figured out server side rendering of custom elements yet, and I don't want that to be a release blocker. Without it, if the custom element doesn't load (due to client side JS being disabled, or other reasons), and if we deduplicate the links, we'd end up with no link to the scrim. The PDF also requires the links, as it just ignores the custom element.

@chrisdavidmills
Copy link
Collaborator

I've made them a little smaller, will deploy the change to staging

@LeoMcA The new size looks great; GTG from my end.

@LeoMcA LeoMcA merged commit f42ad66 into main Sep 30, 2024
2 checks passed
@LeoMcA LeoMcA deleted the scrim-inline branch September 30, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants