Closed dromo77 closed 4 years ago
Hey @dromo77 ,
This is looking great! 😄
I agree that the fixed sidebar feels odd when the sidebar content extends past its container. It also introduces some funky behavior with the sticky header. I'm inclined to remove the fixed behavior of the sidebar. What do you think?
I noticed one other nit: The links in the top level nav don't appear to be centered vertically.
If we can address the fixed behavior and the nav centering I think this is good to go from a design perspective!
I think we should chat about the best way to proceed from there. I know you mentioned that you'd appreciate help on the final dev implementation. It seems like there are a few different ways we could handle this:
One option is to handle this how we would on a client project with defined prototyping and implementation phases. We could treat this as the "prototype" and have a dev make a new branch off master where they recreated your design work, with a focus on accessibility, performance, best practices, etc.
The downside of this approach is we may be reinventing the wheel a bit.
Another option is to have a dev start a branch off of your current branch. They would then go through and make changes to your existing work where necessary.
We could also look through this PR, flag any potential issues, then merge your PR and circle back to resolve those issues afterwards.
When we both have some time today or tomorrow maybe we can sit down and chat about our next steps to see what makes sense?
Thanks!
Thanks for the feedback @Paul-Hebert. It sounds like we're getting close! I think the best way to decide is to remove the fixed positioning so we can get a feel for it. I'll also fix the header alignment.
I agree we should chat through the next steps, my schedule is open 🙂
@dromo77 and I chatted and decided the best way to proceed would be for me to do an initial review of the WIP work.
From there we can catalogue what (if anything) needs to be changed for the final implementation. Once we know that we can determine the best course of action.
@dromo77 , I'll likely be adding some comments to this PR, but don't feel like you need to respond or change anything until we chat in person 🙂
I decided to make a separate branch and remove the dummy patterns and prototypes to make the changes easier to code review: #46
Overall this is looking really good and is close to the final implementation! I think with some changes this will be ready to merge in. :slightly_smiling_face: Because of that I think we should continue to work in the same branch, resolve issues there, and then merge this in! :tada:
There are a lot of comments on the PR linked above, and we should be sure to resolve all of them before merging. Broadly, these can be grouped into a few categories:
EP_
. The reason for this is so that our boilerplate styles don't leak out and affect the patterns end users create. For example, if they create a no-scroll
class, it shouldn't take on the styles of our no-scroll
class. So we call ours EP_no-scroll
. (Danielle)EP_js-
. The js-
part tells future devs that the class is used for JS so they don't change it when making CSS changes. js-
classes should not directly receive CSS styles, so sometimes we need to add redundant classes to elements. e.g. EP_menu-toggle EP_js-menu-toggle
. (Danielle)I'm happy to chat through all of this more if you would like. I think a lot of this is stuff that you're more than capable of resolving, but I'm happy to help as well :slightly_smiling_face:. Let me know if you have any questions. Maybe we can spend a few minutes chatting after you've reviewed to figure out the best way to split this up between the two of us?
Thanks for all your work on this. I'm so excited to merge this into the project! It's looking great!!
P.S. Sorry that the comments on the PR are very terse and aren't very informative. I was initially just writing them as quick notes to myself, but realized afterwards they might be helpful for you as well. I'm happy to expand on any of them.
These changes were merged in a different PR.
Overview
This isn't quite ready for review so I'm creating a draft PR. Most of the stylistic feedback has been addressed. The changes from the last iteration include:
Issues that Need Feedback
To help provide a better idea of how the navigation works with a larger number of items I added some placeholder content (which will need to be removed before a PR). This introduced some new challenges around handling overflow, specifically the sidebar on large screens.
I kept the sidebar fixed, which I think works well on the “Prototypes” overview page where the list is shorter. If the page content is really long and the user has scrolled to the bottom of the page, they don’t have to scroll back to the top to view the sidebar. However, I think it’s less successful on the “Patterns” overview page, where the sidebar overflows. All three big containing elements on the page (header, sidebar, content) have different scrolling mechanisms and it feels a bit odd.
I’m curious what others think about the fixed sidebar.
Next Steps
I'm not sure what changes will be needed for the final dev implementation, but I'd love to get this to a point where it's ready for a PR.