pxt
pxt copied to clipboard
microsoft
Skill map intro page alt text updated to be more descriptive and visible on dark backgrounds
The changes in this PR make it so that alt text for skill map landing page backgrounds is no longer black when the background color is dark. I also added to the descriptions given by the alt text in the InfoPanel so it is specific to an activity and a skill map.
Before
After
Note: There is a check to see how many maps are passed into the SkillGraphContainer component because there can be multiple skill graphs in one landing page for a skill map (i.e. https://arcade.makecode.com/--skillmap#beginner). When this happens, we don't want the alt text to cater to a specific map as the background image applies to the overall experience. Saying background image for skillmap x, y, and z is a bit too much detail to give in alt text, so I thought giving reference to how many skill graphs were represented in the skill map was good detail.
Another Note: Accessibility Insights still gives a flag for no alt text for an image on the usabilla button that we have for feedback. This change is not something that we can access through the iframe we inject on the site. So the following issue won't be fully closed until an alt attribute is added to the image for that button.
Closes https://github.com/microsoft/pxt-arcade/issues/4809
Another thing that I wanted to note: not having the image load/having a broken link does not make the site look like it does in the screenshots that I provided. If a background image is rendered, the skill graphs render according to the size of said background image. This is why you can barely see the skill graphs in the screenshots that I provided. I removed the src of the image tag after the page was completely loaded, thus making the skill graph shrink to it's parent container size. In normal page-loading situations, the image's inability to load would happen before the skill graph renders and thus doesn't actually cause a problem when the site is loaded. I took screenshots in this state because it made it easier to showcase the change.
Probably outside the scope of this PR, but @kiki-lee mentioned this in the original issue: "since skillmap thumbnail images use our special text instead of actual image links".
I don't fully understand what that means, but I was wondering what the motivation was behind doing something custom here. Is that something we've looked into?
Probably outside the scope of this PR, but @kiki-lee mentioned this in the original issue: "since skillmap thumbnail images use our special text instead of actual image links".
I don't fully understand what that means, but I was wondering what the motivation was behind doing something custom here. Is that something we've looked into?
i read that as referring to the src of the content in the skillmap being derived from markdown content that gets parsed into the graph / imgs / etc vs just having html inline or something to that effect - which is as expected, just means we have to fill in the rest of the blanks :)
Probably outside the scope of this PR, but @kiki-lee mentioned this in the original issue: "since skillmap thumbnail images use our special text instead of actual image links". I don't fully understand what that means, but I was wondering what the motivation was behind doing something custom here. Is that something we've looked into?
i read that as referring to the src of the content in the skillmap being derived from markdown content that gets parsed into the graph / imgs / etc vs just having html inline or something to that effect - which is as expected, just means we have to fill in the rest of the blanks :)
Our code to add a level to the skillmap includes a tag for adding the thumbnail ("imageURL:") and there's no place to include alt text.
Probably outside the scope of this PR, but @kiki-lee mentioned this in the original issue: "since skillmap thumbnail images use our special text instead of actual image links". I don't fully understand what that means, but I was wondering what the motivation was behind doing something custom here. Is that something we've looked into?
i read that as referring to the src of the content in the skillmap being derived from markdown content that gets parsed into the graph / imgs / etc vs just having html inline or something to that effect - which is as expected, just means we have to fill in the rest of the blanks :)
Our code to add a level to the skillmap includes a tag for adding the thumbnail ("imageURL:") and there's no place to include alt text.
Ah, got it. Maybe we can file a separate task to add that :) But not needed for this PR. Thanks for the clarification!
Probably outside the scope of this PR, but @kiki-lee mentioned this in the original issue: "since skillmap thumbnail images use our special text instead of actual image links". I don't fully understand what that means, but I was wondering what the motivation was behind doing something custom here. Is that something we've looked into?
i read that as referring to the src of the content in the skillmap being derived from markdown content that gets parsed into the graph / imgs / etc vs just having html inline or something to that effect - which is as expected, just means we have to fill in the rest of the blanks :)
Our code to add a level to the skillmap includes a tag for adding the thumbnail ("imageURL:") and there's no place to include alt text.
Ah, got it. Maybe we can file a separate task to add that :) But not needed for this PR. Thanks for the clarification!
Including the alt-text for a skill map background image seems like it would be a really nice feature. I was looking into it a bit, but I do think it will be a bit more involved and can be separated into a new PR, thanks @thsparks! I will cut an issue.
What is the status of this PR?
I am going to do a pull from master into the branch to make sure that everything is good still, but I should be able to merge this.