search

no-stack-dub-sack / cs-playground-react

In-Browser Algorithm and Data Structures Practice
http://cs-playground-react.surge.sh/
MIT License
520 stars 91 forks source link

Flexbox and Styling Menu Tweaks #54

Closed IsaacAbrahamson closed 6 years ago

IsaacAbrahamson commented 6 years ago

Ok, I decided to just get my changes out of the way so I have free time to work on my school this week. I have some more ideas for the menu and making the site responsive, but this is the first basic set of changes.

Here are the major changes I made:

You can preview the changes at http://playground.isaacabrahamson.com/

Here is a screenshot comparison: image

Hope you like the changes. Either way, I learned a lot making them, and it was fun!

IsaacAbrahamson commented 6 years ago

My solution buttons seem to only work half the time. No idea why this is...

IsaacAbrahamson commented 6 years ago

Oh, looking at console, it's probably my site being on an unencrypted connection and CORS.

no-stack-dub-sack commented 6 years ago

Hey @IsaacAbrahamson, first of all, wow! Thanks for putting in all this effort. At first glance, these changes definitely do add a more modern feel to the app which is really cool, and point duly noted about some of the crowding that's going on in the menu.

That said, before I accept this outright, I'm interested in pulling this down and playing around with it a bit. I have to admit, one of the most difficult things to swallow about open source (though I'm totally open to changes that will improve the project) is letting go of complete creative control  😨

I can most certainly admit when something is better however, and I think if this can be done just right, it will be a huge improvement. I do have a couple of initial concerns though.

EDIT: see this comment which address most of my concerns that I detail below

First, I can see flexbox being a big improvement that will make future styling and responsiveness better, but there are some slight issues with it, that for a detail oriented person like myself, may drive me a bit 🍌🍌🍌 :smile:

For example (and you may not have noticed it on your screen — I don't see it in your screen shot anyway), the challenge names with longer titles affect the orientation of the icons (love this by the way, this + tooltip is excellent), which makes it awkward looking and inconsistent when taken as a whole:

Also, I get wanting to add a little room to breathe, but this feels like too much room maybe? Like its not making use of available space:

It ends up feeling a bit empty and makes me wonder: then why not just make the menu pane itself more narrow by default? (well, I guess because it's resizeable :smile:). That being the case, I think I'd like to see this a bit closer to edge to edge, though I think the padding around the menu items themselves is definitely nice.

Other than that, I think overall this looks really great. What I might like to do is pull it down, play around with it a bit, and see if I can come up with something that we both agree is perfect. What do you think?

Before I do that though, could you do me a favor and add the following to the .gitignore file and update the pr? (just add another commit and push)

.vs/*

This will prevent these VS Code files and binaries from being committed to the repo, but will leave them be in your local workspace:

As a final note, I want to add that, I hope you don't take any of this the wrong way or as undue criticism. I really appreciate the effort and your willingness to work on this! I don't mean to be difficult, but as you can imagine, I've poured quite a bit of time into this project, and do want to make sure I'm 100% happy with any changes. I think this is most of the way there, and love the direction it's going already! You seem to know CSS a bit better than I do, so if you have any ideas on how to fix the flexbox issue, I'm all ears, though I know you're busy with school, so no worries either way!

Thanks again and let me know your thoughts.

no-stack-dub-sack commented 6 years ago

I made some tweaks just with the browsers dev tools, and this is what I think I'd be leaning more towards:

and

Here are the only 2 changes I made:

To be honest, these 2 small changes address almost all of my concerns, there are a few other very slight details that I might like to play around with still, but these spacing issues were the 2 primary (and like this, the icon orientation thing would only even happen on much smaller screens, so could be safely pushed to the back burner).

I think it looks super awesome like this! Are you firm in thinking the other way is better? If so, I'm open to hearing it, because if there's a solid UI Design concept behind it or something, I'm open to reason.

IsaacAbrahamson commented 6 years ago

Hey @no-stack-dub-sack, thanks for the detailed review!

have to admit, one of the most difficult things to swallow about open source (though I'm totally open to changes that will improve the project) is letting go of complete creative control 😨

Agreed 😆

For example (and you may not have noticed it on your screen — I don't see it in your screen shot anyway), the challenge names with longer titles affect the orientation of the icons (love this by the way, this + tooltip is excellent), which makes it awkward looking and inconsistent when taken as a whole:

Glad you like the icons! Yes, I did notice that at first, but then forgot about that. I was planning on fixing that when I made sure the whole thing looked fine < 500px for mobile screens. Like you mentioned later, it shouldn't be too big of an issue if you expand the menu width.

That being the case, I think I'd like to see this a bit closer to edge to edge, though I think the padding around the menu items themselves is definitely nice... I made some tweaks just with the browsers dev tools, and this is what I think I'd be leaning more towards... I think it looks super awesome like this! Are you firm in thinking the other way is better? If so, I'm open to hearing it, because if there's a solid UI Design concept behind it or something, I'm open to reason.

I think a little whitespace around the edges is good, but I see your point about having more room. I'll take a look at it again. I respect your opinion that 60% is a little low while the margins are a little high. After all, it's your project so it should look the way you like. I'll try to make it more like that and compromise, maybe 85-95% depending on the width. See how that goes 😄

Wonder if there's a way to style these? do they accept a className prop or style object that gets passed down to the underlying div or whatever it is under the hood I wonder? I'll have to check out the docs. Might be cool to style these to match the themes, or is that too much do you think? The stark white on black contrast is def really nice as it is.

I'll let you read the docs on that. The little bit I did there pushed the limits of my React capabilities 🤣. It was really cool though seeing it work. I'll have to go back to my React tutorials once I have more time. I think you should take care of that as I'm not sure I can...

As a final note, I want to add that, I hope you don't take any of this the wrong way or as undue criticism. I really appreciate the effort and your willingness to work on this! I don't mean to be difficult, but as you can imagine, I've poured quite a bit of time into this project, and do want to make sure I'm 100% happy with any changes. I think this is most of the way there, and love the direction it's going already! You seem to know CSS a bit better than I do, so if you have any ideas on how to fix the flexbox issue, I'm all ears, though I know you're busy with school, so no worries either way!

Of course not. I want you to be happy with it. I'll look at the flexbox thing again. My original thought was a media query, but I'll have to look into it more this weekend. Thanks for the help and ideas 💯

IsaacAbrahamson commented 6 years ago

After thinking about this, the flexbox shouldn't be too difficult of a fix. I'll add a container for the buttons and tweak flexbox settings. Maybe might not even need a media query. I'll push some commits later with the gitignore and width modifications.

IsaacAbrahamson commented 6 years ago

Ok, I made some changes. Gitignore vscode files, and I modified the spacing like you requested.

Check it out: http://playground.isaacabrahamson.com/

I like it like this. There isn't as much space, and it is more like your original design. That said, there still is some whitespace on the edges, but I think there's only enough there to make the whole menu spacing look consistent on all sides.

I'll get to the flexbox here soon, just wanted to get that out of the way so you can clone and try it out yourself.

IsaacAbrahamson commented 6 years ago

Ridiculously stupid fix. I have no idea why it worked but it did.

Also had to tweak the max-width of the text because some of the longer words like "checkerboard" that wouldn't break to a new line were screwing up the icons.

Looks perfect down to about 270px. I have no idea why it would need to go any smaller than that... I'm guessing for mobile devices, you would have a hamburger button to pull the menu out full-width of the device. The smallest screen I can think of is at least 300px so everything should be fine there!

Give it a try locally, and tell me what you think! I hope you can get the solution buttons to work as I have no idea. 😢

IsaacAbrahamson commented 6 years ago

Here is what an iphone 8 looks like without the scrollbars:

no-stack-dub-sack commented 6 years ago

As for your changes, this all sounds good! Let me take a closer look when I get home, getting a bit too distracted at work :smile:

IsaacAbrahamson commented 6 years ago

Ok, let me know when your staging branch is ready and I'll send another PR. Just curious, can't you rollback the master to a previous commit?

IsaacAbrahamson commented 6 years ago

Also, I don't need to make a new PR, I can edit this to staging at the top.

no-stack-dub-sack commented 6 years ago

@IsaacAbrahamson

Just curious, can't you rollback the master to a previous commit?

Not always that simple, I wish it were. Once additional commits are made on top of that, it's get's more complicated. When it's just one PR, yes, you can revert that PR. To be honest, I haven't tried that too much, but I think it's safer not to start :-)

Also, I don't need to make a new PR, I can edit this to staging at the top.

True, but then the vscode files will still be committed as they are on your remote branch no matter what at this point. This is why I think fixing the .gitignore and pushing to a new remote branch will keep those files out of git on the remote branch you make a PR from.

no-stack-dub-sack commented 6 years ago

@IsaacAbrahamson Ok, staging is now up to date with master if you want to create a new branch and redo the PR. I think .vs/** should do it in the gitignore. I don't use vscode, but .vs is a folder, right?

no-stack-dub-sack commented 6 years ago

Looks pretty solid on an iphone, and the flexbox fix is great, works really well on all screen sizes like you said, funny how simple fixes can be sometimes!

I agree, too, that if ever optimized for mobile, the solution you mentioned would be best, so I don't think we really need to accommodate the full menu split panes on very small screens.

We can continue the convo on the new PR, but here's my thing with the top and bottom margins on the details elements:

When it's all closed up, we still have 3 different sized gaps between the different drawers. If we only use the top margin, then it's always consistent. I tend to think 10px is enough.

Here's where I'm feeling the sweet spot is. The white space around the menu edges is starting to grow on me, but I think a hint of it is enough to have the desired effect and not leave the eye wanting more. This is with a top margin of 10px on the details elements, no bottom margin, and 96% width on .sidebar--menu>details. What do you think?

Lastly, "solution" issue solved! I didn't realize they were SVG elements. This is what they look like inside:

image

When any of the inside lines are clicked (which don't have the necessary id to trigger the solution), nothing gets populated in the editor. I should be able to fix this pretty easily by using e.currentTarget.id instead of e.target.id to make sure the event gets bound correctly to the right element. Try clicking the spine of the open book - same thing, the modal won't open.

no-stack-dub-sack commented 6 years ago

Here's a couple of other things I've noticed that I don't want to forget so just tracking here:

no-stack-dub-sack commented 6 years ago

@IsaacAbrahamson Dude! This thread is getting a little long, but have to mention this!! I've been trying to figure out for forever what happened to bracket matching in the editor (like highlighting matching brackets). It worked at first and then all of a sudden it was gone. Not sure what you did, must have been some CSS rule that was overriding it, but, miraculously it works again!!! Yessssss

image

IsaacAbrahamson commented 6 years ago

Closed in favor of https://github.com/no-stack-dub-sack/cs-playground-react/pull/55