Closed dancormier closed 3 months ago
Name | Link |
---|---|
Latest commit | 1524156e5b179175d6a76be6c3aef66dca077540 |
Latest deploy log | https://app.netlify.com/sites/stacks/deploys/65fdab8baf05c1000835098c |
Deploy Preview | https://deploy-preview-1670--stacks.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
@dancormier Yup! I have a note in the ticket (last bullet) but basically remove the entire section from that page since it will now live on the new page.
I also have some left nav hierarchy changes mentioned in the design specs β open to feedback on these.
Great work Dan. Looks good overall. π
I am happy to merge it in although I am not entirely sold on the
stacks-markdown
plugin as it is now. I understand that it will save us from repeating classes and tags over and over but I dislike that we need to open and close shortcodes and we lose syntax highlighting in the process. I made a suggestion about exploring to run the converter you wrote against the whole markdown found in the view file without the usage of shortcodes (see review comment).Also in my opinion we should make an effort to test that converter even if it's just for our docs. I can see how we want to change/add things there over time and having test cases will help us to do that with the confidence of not breaking things. Let me know if you need support setting those tests up (I would recommend to use vitest if you want to give it a shot - e.g.
"test:docs:unit": "vitest path/to/docs"
) π
You're the voice of reason here, @giamir. I've removed the stacks-markdown
from this PR (in commit https://github.com/StackExchange/Stacks/pull/1670/commits/46e0cdbfdeac6d570625ef084a0b709c86938a10) so we can keep this PR scoped reasonably and get the accessibility page merged without hesitation.
FWIW, I think you're right to not be totally sold on the stacks-markdown
plugin. I think we can explore it further when the time is right. For now, I've ported the plugin to the dcormier/docs-markdown-support branch so we have the code if/when we feel like revisiting.
Let me know if this seems reasonable.
Thanks for the review @CGuindon. I've made changes that should address the issues you've found.
Did a quick visual pass over:
- The link is correct but I just realized our copy had a typo, the HC link should for " Success Criterion 1.4.3 Contrast (Enhanced)" should "1.4.6"
![]()
β Fixed in https://github.com/StackExchange/Stacks/pull/1670/commits/04c76bc9022d1b52c1d414e0f2f0ebdf7583c6b9
- Any chance we can easily get the color swatches to be vertically aligned in the middle? If it's too annoying, I can live with it.
![]()
β Fixed in https://github.com/StackExchange/Stacks/pull/1670/commits/55ee4694d351e9c2511d75fb873cc9f064597ea2
- I noticed the little text labels on the images disappear when I re-size the screen to small, is that on purpose/normal?
![]()
The disappearing labels are a remnant from the Color Fundamentals page build. Since the color rows in the Usage section shift to a vertical orientation in smaller viewports, it didn't make sense to show those labels in that partial. It also just kinda helped sidestep having to deal with ensuring the examples wrapped/flowed gracefully.
I've changed the partial to show those labels at all screen sizes unless we override it in https://github.com/StackExchange/Stacks/pull/1670/commits/087774b38a93c2a92dc5f88e366a896386fbc681. Let me know how this looks to you. I'm happy to make modifications if you think we should.
- Left nav changes and moving the Current Color section
β Completed in https://github.com/StackExchange/Stacks/pull/1670/commits/e51495d3f7723d06d48ffea15290366e2424bc49
FWIW, I think you're right to not be totally sold on the stacks-markdown plugin. I think we can explore it further when the time is right. For now, I've ported the plugin to the dcormier/docs-markdown-support branch so we have the code if/when we feel like revisiting.
It works for me. Maybe you can also create a quick companion "operational maintenance" jira ticket if it makes sense (it will probably end up in the low priority pile but at least we don't lose track of the improvement). Thank you. π
@dancormier @CGuindon Is the Accessibility navigation item under the correct category? https://deploy-preview-1670--stacks.netlify.app/product/guidelines/accessibility/
@dancormier @CGuindon Is the Accessibility navigation item under the correct category? https://deploy-preview-1670--stacks.netlify.app/product/guidelines/accessibility/
Yep, I originally missed the reorganization of that navigation in the Figma and made those changes in https://github.com/StackExchange/Stacks/commit/e51495d3f7723d06d48ffea15290366e2424bc49
@dancormier @CGuindon Is the Accessibility navigation item under the correct category? https://deploy-preview-1670--stacks.netlify.app/product/guidelines/accessibility/
Yep, I originally missed the reorganization of that navigation in the Figma and made those changes in e51495d
@giamir @dancormier I also wanted to change the name of that top section to "Develop" β since everything in here is a Guideline. Thoughts or concerns?
@CGuindon Updated!
Commented on this commit directly since I can't push any changes (permission thing I think?). Otherwise the other changes look good.
I also wanted to change the name of that top section to "Develop" β since everything in here is a Guideline. Thoughts or concerns?
@CGuindon I think that'll work fine. I originally changed the wrong heading but have fixed it in https://github.com/StackExchange/Stacks/pull/1670/commits/d092389d26a14810a4e2fa89070c2159dd723641
I made another PR to update urls to better reflect the nav structure. This isn't entirely necessary but it feels like a good excuse to perform that cleanup. @giamir @CGuindon when you get a moment, let me know what you think of that PR and I'll either merge it into this PR or close it out.
STACKS-472 This PR adds an accessibility page to our docs site.
Question for @CGuindon: Should we make any changes to the Color fundamentals > Accessibility standards section?
TODO
BASE
section