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

Restructure layout of playground #1384

Merged
merged 2 commits into from Jul 17, 2023
Merged

Restructure layout of playground #1384

merged 2 commits into from Jul 17, 2023

Conversation

contificate
Copy link
Contributor

Commit message:

This set of changes seeks to address issue #1381.

  • Add split.js splitter between code editor and output pane. This is somewhat a novelty feature, but I believe it could prove useful on limited displays, also permitting the easy extension of other features (such as a future settings window, which could be configured to hide to make most use of display real estate).
  • Address issue where text disappears beneath the bar containing the "Run" button. Previously, this button was placed absolutely to account for the difficulty of laying codemirror out in a flex layout. I believe this is now resolved.
  • Address issue where full page height is not being utilised. This is achieved by flexing the entire body of the playground page, so the content area can sit below it, flexed.
    The CSS additions have been put into a fresh asset file. The reason being that I haven't yet gotten around to considering if there's any reasonable conflicts or redundancies with codemirror.css.

I've tested these changes and believe them to work on every major browser. I'm hoping someone can review the changes to get a feel for the difference. The only really opinionated thing is the layout now exists within a split pane (provided by https://split.js.org/ - MIT License, simple) to allow for some flexibility on limited displays.

My hope is that, with these changes, room for addressing other issues - which may involve the addition of extra UI elements - has been made.

Please be sure to flag up any layout related issues you encounter.

Copy link

@StonedHesus StonedHesus left a comment

Choose a reason for hiding this comment

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

I approve of @contificate's changes to the playground’s layout. The code seems to be introducing the desired effects without altering any other aspects of the view, thus ensuring that all the previous functionality remains unaltered and usable, moreover it is finally addressing the bug which always placed the Find & Replace widget out of view, hence, hopefully, aiding in providing a more contoured UX.

@contificate contificate marked this pull request as ready for review July 7, 2023 18:06
@sabine
Copy link
Collaborator

sabine commented Jul 12, 2023

Thanks so much @contificate and @StonedHesus, this is looking really good! Uncovering the Find/Replace UI is super useful - I didn't even know that existed! 👍 Sorry I didn't get to review it properly yet, I'm still saying "yes" to too many things, so everything is busy at the moment. 🙂

There's a few things I need to think through, namely

  • what can we do about the mobile view for the playground - I think we're so close to achieving that now. (It looks to me like the original plan here was to have one tab for the editor, and another tab for the output - however, I think a good UX is more complex than that.)
  • in context of the splitjs library, if we can "enable/disable" it when the window size changes between mobile view and regular view

Besides that, when we decide to go with splitjs, I'll vendor the minified js into the repository. (It's a policy and it means that having JavaScript from external sources disabled won't break the layout.)

Regardless, this is a great improvement, and we can still look at the mobile view at another time. I hope to merge this by end of this week or early next week.

@StonedHesus
Copy link

@sabine I appreciate you taking your time to thank @contificate and I for pointing this here UI issue, though the credit ought to be attributed solely to @contificate since he was the one who ended up implementing the actual solution to the UI problem.

As for reviewing the code changes yourself, do bide your time with it, for I am quite sure that it isn't that pressing of a matter.

Now when it comes to the UI for mobile platforms, I think I have some suggestions, we can discuss more in the days and weeks to follow over Discord, but in the meantime I think that we ought to open an issue for that, that is if there isn't one already opened. And, in addition to that, we also need to establish how we can collaborate on the design process directly in Figma, so as to agree upon layout modification prior to implementing them in the codebase.

@sabine
Copy link
Collaborator

sabine commented Jul 13, 2023

I happened to find some time this morning.

Since there was some trouble with the mobile layout and splitjs, I removed it in favor of the old 60/40 split.

I also moved all the CSS into the playground.eml template as Tailwind classes.

I think this is in a good shape to merge like this. I opened #1391 for the task of creating mobile view. Regarding Figma, I have to check on our designer and see where we will have a space to co-design and review designs.

contificate and others added 2 commits July 13, 2023 11:10
This set of changes seeks to addres issue #1381.

- Add split.js splitter between code editor and output pane. This is
  somewhat a novelty feature, but I believe it could prove useful on
  limited displays, also permitting the easy extension of other features
  (such as a future settings window, which could be configured to hide
  to make most use of display real estate).

- Address issue where text disappears beneath the bar containing the
  "Run" button. Previously, this button was placed absolutely to account
  for the difficulty of laying codemirror out in a flex layout. I
  believe this is now resolved.

- Address issue where full page height is not being utilised. This is
  achieved by flexing the entire body of the playground page, so the
  content area can sit below it, flexed.

- Ascribe an identifier, "output-area", to the relevant element to be
  scrolled in a future feature addition.

The CSS additions have been put into a fresh asset file. The reason
being that I haven't yet gotten around to considering if there's any
reasonable conflicts or redundancies with codemirror.css.
@sabine sabine added the testers welcome Not sure what to do? Maybe help us out: run this PR and check if everything works as intended! label Jul 13, 2023
@sabine sabine merged commit 7cb8987 into ocaml:main Jul 17, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testers welcome Not sure what to do? Maybe help us out: run this PR and check if everything works as intended!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants