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

Add Bramble API for toggling whitspace view, part of mozilla/thimble.mozilla.org#1763" #664

Closed
wants to merge 19 commits into from
Closed

Conversation

tonypark78
Copy link

Refer : thimble-issue-1763
Implemented white-space-indicator

@humphd
Copy link

humphd commented Mar 27, 2017

@tonypark0403 can you please Edit (there's a button at the top) this PR's topic line so that it says more than "Thimble issue1763"? I'd suggest, "Add Bramble API for toggling whitspace view, part of https://github.com/mozilla/thimble.mozilla.org/issues/1763"

@humphd
Copy link

humphd commented Mar 27, 2017

Also, is this ready for a review? If so, let me know.

@flukeout
Copy link

Hi @tonypark0403 this is coming along great! I left a couple of comments in the Thimble side of this PR mozilla/thimble.mozilla.org#1914

@tonypark78 tonypark78 changed the title Thimble issue1763 Add Bramble API for toggling whitspace view, part of mozilla/thimble.mozilla.org#1763" Mar 28, 2017
@@ -44,8 +44,8 @@ define(function (require, exports, module) {

// --- Settings ---

var commandID = "denniskehrig.ShowWhitespace.toggle";
var preferencesID = "denniskehrig.ShowWhitespace";
var commandID = "allowWhiteSpace.toggle";
Copy link

@Pomax Pomax Apr 3, 2017

Choose a reason for hiding this comment

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

@humph does this also require turning off whatever "denniskehrig.ShowWhitespace" is in some other part of the code? (extension loader or some other place?)

Copy link
Author

Choose a reason for hiding this comment

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

@Pomax , I am testing, I will replace it again.

Copy link

Choose a reason for hiding this comment

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

I'd like to see these strings get changed back to "denniskehrig.ShowWhitespace" and ""denniskehrig.ShowWhitespace.toggle", since we're using someone else's code, it seems odd to just write them out of the picture here.

These are defining the namespace for the extensions prefs and command id, and this is the only place where they get defined. Elsewhere we'll need to use them in order to call the code.

@humphd
Copy link

humphd commented Apr 5, 2017

@tonypark0403 #678 is another PR doing similar work, if you want some advice about maintaining your state. @hkirat has got it working.

@tonypark78
Copy link
Author

I checked every files you recommended then I couldn't figure out what was different from what I did. That's why now I deleted my repository and re-forked from upstream. I think I need more training for git and work together since my previous one had a lot of error even mozilla chat said just re-pull and restart again, which is very good for me because I was trained to implement code by instruction but this time no instruction......

@humphd
Copy link

humphd commented Apr 6, 2017

OK, let us know if you still need help. I think you're very close to getting this.

@tonypark78
Copy link
Author

And re pull and restart......... I did it more than 10 times and every time it showed me different coding position that made me confused a lot.
Also, I checked the bramble client main.js and StateManager?.
But still I have no idea because the files only some variables are assigned. I don't know where the files come from.

@tonypark78
Copy link
Author

And can you resend about the mail you mentioned on Tuesday? I cannot find the mail.
I got 7 mails from you but I could find it. Sorry.

@humphd
Copy link

humphd commented Apr 6, 2017

See my comment #588 (comment)

@tonypark78
Copy link
Author

I am not asking about resetting bramble but rebating or keeping uptodate files.
I am working in the branch, issue-xxxx.
If I am behind the upstream's master, git fetch upstream and then git rebase -i master in issue-xxxx.
is it correct?
Thank you for helping me to understand git.

@humphd
Copy link

humphd commented Apr 6, 2017

If you have a branch, issue-xxxx, and need to update it so it includes what's on master, so the following:

git checkout master
git pull upstream master
git checkout issue-xxxx
git rebase master

In this case you don't need to do an interactive rebase (that's what -i does), since you want all of your commits to get moved from the current base commit (where you originally created your branch) to the new master commit.

If you get yourself into a lot of trouble in git, and need to correct a mess, another thing you can do is to cherry-pick specific commits onto a new branch from the old one.

@tonypark78
Copy link
Author

This is closed and this is the new PR.

@tonypark78 tonypark78 closed this Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants