Skip to content
This repository was archived by the owner on Jun 14, 2018. It is now read-only.

Changed watch task according to feedback #145

Closed
wants to merge 5 commits into from

Conversation

s-KaiNet
Copy link
Member

Solves #49

andrewconnell and others added 4 commits January 28, 2016 05:37
Updated tests to address issue where jQuery was being loaded before Angular and thus, Angular was using jQuery & not jqLite, causing false positives.

Closes ngOfficeUIFabric#123.
Renamed and combined tasks.
Updated description.
Alpha- sort.
Prettify help output.
@s-KaiNet
Copy link
Member Author

One step closer to perfection :).
Added changes based on latest conversation

@andrewconnell
Copy link
Member

Looking good! Checking out the watch and live dev stuff now, but wanted to mention this other part...

The new formatting of the help makes it very hard to read. Adding a newline between every single option makes reading the options that go with the task quite unreadable. Can we address / revert this? Previously I mentioned that I thought we should wrap long lines & indent them so things don't wrap. the task we're using to build the help only indents options 1 space under the task... when you add a lot of vertical space, that indentation is hard to read.

IMHO, there is wrapping that doesn't need to be there. For instance, the build-lib --verbose doesn't need to be wrapped as it's under 120 char long for the whole line.

Here's what I see (also tried in the stock terminal window at 80 & 120 char wide):
screen shot 2016-01-31 at 6 04 21 am

As you can see, I'm having to scroll a good bit up to see the first few tasks when I run help. IMHO this isn't necessary and we need to remove all this wrapping & additional extra vertical space. The only thing we want to address are descriptions that wrap. For instance, here's a screenshot of how it was. Notice the build-lib --dev and a few within the watch task wrap... those are the only ones that really need changes.

screen shot 2016-01-31 at 6 08 30 am

@andrewconnell
Copy link
Member

BTW, everything else looks great! Let's tweak the way the help is displayed as this is a pretty big change and then I think we're good.

@s-KaiNet
Copy link
Member Author

Hmm... Ok, so I remove extra line breaks for task description and add line breaks only if description length more than 120 symbols.
I don't think we can have a good looking help for all cases, because, as for me, for example, I'm using 80 char wide console, and for description it takes only about 60 symbols. if description is more than 60 wraps occur and it doesn't look pretty. Also I'm not using such a tall console window and always need to scroll up to see the first tasks. I don't know how about others, but in your case it will good only for certain scenarios. Is it OK?

@andrewconnell
Copy link
Member

Totally agree we can’t make it great for every case & 120 isn’t a hard & fast rule… I’m cool with 80 because I get many take the default width which is 80… so wrapping at 80 seems pretty standard. For me the information density with so many line breaks & empty lines just made it hard to consume.

@ghost
Copy link

ghost commented Feb 1, 2016

Awesome job guys! Can't wait to rebase my repo with these changes :) The fact that with tslint issues the specs don't run is blocking me developing.

@s-KaiNet
Copy link
Member Author

s-KaiNet commented Feb 1, 2016

One more attempt :).
Here is how it looks now:
2016-02-01_2112

@andrewconnell
Copy link
Member

Awesome... looks great! Adding to my list of things to merge.

Closes #49

andrewconnell pushed a commit that referenced this pull request Feb 1, 2016
Rename `watch` task to `live-dev`.

Update `live-dev` task to (1) perform first pass of all things before watching for changes; (2) to only run tests within the test file that was updated; (3) never fail on errors in any checks (vet, test, build)... always run each of these; (4) optionally use browsersync to do live dev of demos with the new `--serve` flag.

Due to big changes to build process, when updating forks, recommend following process to clean out all old JavaScript from old tasks:

```
gulp clean
tsc -p ./
```

Closes #49. Closes #145.
@andrewconnell
Copy link
Member

Manually merged to dev, Closes #145

@s-KaiNet
Copy link
Member Author

s-KaiNet commented Feb 1, 2016

Ihaaa! Merged! :))
Thank you!

@s-KaiNet s-KaiNet deleted the issue-49 branch February 1, 2016 20:01
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.

2 participants