Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

More fully fleshed out UX #27

Merged
merged 29 commits into from
Nov 28, 2017
Merged

More fully fleshed out UX #27

merged 29 commits into from
Nov 28, 2017

Conversation

kitsonk
Copy link
Member

@kitsonk kitsonk commented Oct 27, 2017

Enhancement

This PR more fully fleshes out the designed Workbench as well as makes various UI/UX improvements and maintainability improvements.

@codecov
Copy link

codecov bot commented Oct 27, 2017

Codecov Report

Merging #27 into master will decrease coverage by 1.39%.
The diff coverage is 57.35%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #27     +/-   ##
=========================================
- Coverage   67.96%   66.56%   -1.4%     
=========================================
  Files          23       17      -6     
  Lines        1414     1002    -412     
  Branches      332      239     -93     
=========================================
- Hits          961      667    -294     
+ Misses        453      335    -118
Impacted Files Coverage Δ
src/main.ts 100% <100%> (ø) ⬆️
src/Workbench.ts 48.88% <56.71%> (-1.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e22caba...7547768. Read the comment docs.

@kitsonk kitsonk added this to the 2017.10 milestone Oct 30, 2017
@dylans dylans modified the milestones: 2017.10, 2017.11 Nov 1, 2017
@kitsonk kitsonk requested a review from nicknisi November 2, 2017 14:52
Copy link
Member

@nicknisi nicknisi left a comment

Choose a reason for hiding this comment

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

Overall, this looks great! I tested in Chrome, Safari, and Firefox. Here's a couple of smaller issues I've noticed.

  • This bar for the scrollbar appears as soon as anything is typed into the text editor. Not causing any issues, just looks a bit odd and doesn't go away.

napkin 11-02-17 1 04 26 pm

  • The editor doesn't resize vertically when the window is resized

napkin 11-02-17 1 11 34 pm

  • The default editor that opens with the page can't be closed or removed. This isn't causing issues, but is it expected behavior? I'm not sure what it should really do, honestly.
    default-editor

  • The editor can't be easily scrolled horizontally in Chrome or Safari, or at all in Firefox.
    horizontal-scroll

position: relative;
height: 100%;
width: 100%;
width: 100%
Copy link
Member

Choose a reason for hiding this comment

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

missing semicolon

@kitsonk
Copy link
Member Author

kitsonk commented Nov 27, 2017

@nicknisi I have tried to address most of the feedback:

  • There is no way to squash the navigation based on scroll, so I have added an event listener to the Workbench that asks if the user wants to navigate away.
  • Added an event listener for window resize events which causes the editor to layout a bit better when resizing the window. It will "flicker" though, which is just life at the moment I am afraid.
  • Changed it so that the editor is "hidden" when nothing meaningful is being displayed. Therefore when all the files a closed, there is no editor, which better deals with a "phantom" editor that doesn't do anything.

It would be good to try to land this, so I can rebase my live editor and you can rebase your stuff and we can iterate any further in future PRs.

Copy link
Member

@nicknisi nicknisi left a comment

Choose a reason for hiding this comment

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

I made a comment about the event listeners, but once you've reviewed that it looks good to me! 👍

return evt.returnValue;
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to clean up these event listeners when the workbench is unloaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the 0.2.0 there are better lifecycle methods that would make this possible, so let's open an issue to clean that up when we move to 0.2.0 (which will be my next PR).

@kitsonk kitsonk merged commit 22cf197 into dojo:master Nov 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants