added fullscreen button for notes and snippets by nadr0 · Pull Request #602 · BoostIO/BoostNote-Legacy

nadr0 · 2017-05-28T01:39:42Z

allows user to hide the sidebars for fullscreen editing, there is an expand button next to the trash ca., You press it to expand to fullscreen and go back to your old view. Images added below.

I have never used this stack before so I don't quite fully understand react+redux so the code falls back to the dom to edit the divs.

Anyone point me in the right direction to clean the handleFullScreenButton() call?

I would want a hide() function on certain react components? For I could just call those like

handleFullScreenButton (e) {
// somehow have access to these objects?
if(this.state.fullScreen.status) {
    noteDetail.hide()
    mainBody.hide()
    sliderRight.hide()
    slider.hide()
else {
    noteDetail.show()
    mainBody.show()
    sliderRight.show()
    slider.show()
}
}

asmsuechan

Thank you for your great PR! 😄
To begin with, could you fix some pointed by eslint?

asmsuechan · 2017-05-28T02:33:20Z

browser/main/Detail/MarkdownNoteDetail.js

+    const noteDetail = document.querySelector(".NoteDetail")
+    const mainBody = document.querySelector(".Main__body___browser-main-")
+    const sliderRight = document.querySelector(".Main__slider-right___browser-main-")
+    const slider = document.querySelector(".Main__slider___browser-main-")


Those class names such as .Main__slider___browser-main- depends on the specification of webpack, so could you name any other one to them?

I had to give them Ids because they don't have any other classes on them

asmsuechan · 2017-05-28T02:33:41Z

browser/main/Detail/MarkdownNoteDetail.js

+        status: false,
+        oldState: {
+          nd : 0,
+          mb : 0


Could you change the names? It's hard to understand.

You can simply change to them.

fullScreen: false, widthOfNoteDetail: 0, widthOfMainBody: 0

asmsuechan · 2017-05-28T02:34:27Z

browser/main/Detail/MarkdownNoteDetail.js

+            <button styleName='control-fullScreenButton'
+              onMouseDown={(e) => this.handleFullScreenButton(e)}
+            >
+              <i className={'fa fa-arrows-alt'} styleName='fullScreen-button' />


Probably, you can simply use 'fa fa-arrows-alt'.

err, you still have to leave the fa in the front.

Sorry, could you explain the reason more clearly? It works fine at least in my environment (MacOS).

haha totally tunnel visioned what you said and didn't realize you mean take off the {}

you mean take off the {}

Yes 🙋‍♀️

asmsuechan · 2017-05-28T03:18:20Z

browser/main/Detail/MarkdownNoteDetail.js

+        status: false,
+        oldState: {
+          nd : 0,
+          mb : 0


You can simply change to them.

fullScreen: false, widthOfNoteDetail: 0, widthOfMainBody: 0

asmsuechan · 2017-05-28T03:18:25Z

browser/main/Detail/MarkdownNoteDetail.js


+  handleFullScreenButton (e) {
+
+    this.state.fullScreen.status = !this.state.fullScreen.status


You should use this.state.setState().

const currentScreenState = Object.assign(this.state.fullScreen, {status: !this.state.fullScreen.status}) this.setState({ fullScreen: currentScreenScreen })

nadr0 · 2017-05-28T06:28:38Z

eslint is now passing but for some reason a linux test case is now failing when it didn't before.

asmsuechan · 2017-05-28T06:46:24Z

browser/main/Detail/MarkdownNoteDetail.js

  }

+  handleFullScreenButton (e) {
+    const currentScreenState = !Object.assign({}, this.state).fullScreen


Now that you don't have to use Object.assign (I commented that before I offer you to change the structure of the state).
You can simply write this.setState({ fullScreen: !this.state.fullScreen }).

gotcha, I ended up having to wrap the show/hide code inside the callback of the setState. It wasn't being updated properly so I read the react docs and found " React does not guarantee that the state changes are applied immediately.".

asmsuechan · 2017-05-28T06:47:48Z

browser/main/Detail/SnippetNoteDetail.js

+      fullScreen: false,
+      widthOfNoteDetail: 0,
+      widthOfMainBody: 0
+


extra line 😎

asmsuechan · 2017-05-28T06:49:53Z

The test occasionally fails 😨

asmsuechan · 2017-05-29T01:30:10Z

browser/main/Detail/MarkdownNoteDetail.js

+      isLocked: false,
+      fullScreen: false,
+      widthOfNoteDetail: 0,
+      widthOfMainBody: 0


Alright, let's move on to the architectural review!
First of all, these states should not be placed the component because they're out of the role of it.
I think browser/main/Main.js should manage them rather than MarkdownNoteDetail because those states are overall states for Boostnote, and you'll have to use eventEmitter to call handleFullScreenButton() from MarkdownNoteDetail.
I believe this PR (https://github.com/BoostIO/Boostnote/pull/283/files) is helpful for you!

If you don't make out what I say (my English), feel free to ask me anything 😄

Yeah I was concerned about that from the start. I ended up moving the logic/event into browser/main/Main.js like you suggested. I just have emit's inside the MarkdownNoteDetail and the snippet file. I tried to follow the code from the PR you recommend.

Also, thanks for all the help!

asmsuechan · 2017-05-30T05:31:56Z

browser/main/Main.js

+      mainBodyWidth: 0
    }
+
+    this.fullScreenEditorCode = () => this.handleFullScreenButton()


I prefer this.toggleFullScreen to this name.

asmsuechan · 2017-05-30T05:47:31Z

browser/main/Main.js

+        sliderLeft.style.display = 'block'
+      }
+    })
+  }


This is the last step (probably 😉). Let's change some codes in order to easy to maintain. To begin with, you can remove variables. I assume slideRight and slideLeft is not required, only noteDetail and mainBody is needed in this method.
Next, you can separate this method into 3, show, hide, and handleFullScreenButton.

handleFullScreenButton (e) { const noteDetail = document.querySelector('.NoteDetail') const mainBody = document.querySelector('#main-body') this.setState({ fullScreen: !this.state.fullScreen }, () => { if (this.state.fullScreen) { this.hideLeftLists(noteDetail, mainBody) } else { this.showLeftLists(noteDetail, mainBody) } }) } hideLeftLists(noteDetail, mainBody) { this.state.noteDetailWidth = noteDetail.style.left this.state.mainBodyWidth = mainBody.style.left noteDetail.style.left = '0px' mainBody.style.left = '0px' } showLeftLists(noteDetail, mainBody) { noteDetail.style.left = this.state.noteDetailWidth mainBody.style.left = this.state.mainBodyWidth }

And I would like to have your opinion. Do you think the names of each method proper? I think they're not the best.

sliderRight and sliderLeft are important because if you are in the editor mode, the slider bars to move the lists appear through the editor. You would be able to drag those sliders which is not good. I need to turn those off or display none them while you are in fullscreen.

I do like how you separated the code into 3 functions. I do not like the name leftLists, it is not descriptive. I do not quite know what to call them. If they have different names we should use them? I just called them lists because I don't know what else to call them.

I cannot reproduce the behavior. Could you tell me more detail?

How about showListColumns() and hideListColumns()?

If you scan your mouse left to right very slowly in the edit mode. You should be able to find the vertical sliders that allow you to move the lists left and right. That is if you turned off the slider code, so that is why I did display: none.

I think those names are better, columns make more sense.

I got it. I think the problem is solved by css. z-index might be helpful. You can apply z-index to those sliders and NoteDetail.

Are you saying edit the CSS files or do it dynamically in the javascript? I can do this tomorrow.

Please edit the CSS 😄
Thank you so much!

asmsuechan · 2017-06-03T06:04:34Z

@nadr0 I fixt the broken test, so can you rebase the current master branch?

nadr0 · 2017-06-04T17:28:42Z

I don't think the rebase ended up working?

asmsuechan · 2017-06-05T06:10:49Z

@nadr0 Please rebase upstream/master into this branch.

$ git remote set-url upstream git@github.com:BoostIO/Boostnote.git # if you don't set
$ git fetch upstream
$ git checkout master
$ git rebase upstream/master
$ git push -f origin HEAD

allows user to hide the sidebars for fullscreen editing

asmsuechan · 2017-06-08T07:06:31Z

@nadr0 You have to delete the merge commit.

$ git branch -b master-backup # just in case
$ git checkout master
$ git reset --hard HEAD^
$ git push -f origin HEAD

nadr0 · 2017-06-10T21:44:04Z

my git skills aren't up to par 😓

asmsuechan · 2017-06-12T08:22:04Z

@nadr0 Leave it to me.

asmsuechan · 2017-06-12T08:35:43Z

@nadr0 Finally, I decided to handle by using cherry-pick. So I close this PR.
#632

asmsuechan suggested changes May 28, 2017

View reviewed changes

asmsuechan reviewed May 28, 2017

View reviewed changes

asmsuechan suggested changes May 29, 2017

View reviewed changes

asmsuechan suggested changes May 30, 2017

View reviewed changes

nadr0 added 13 commits June 7, 2017 20:14

added fullscreen button for notes and snippets

26ebfc5

allows user to hide the sidebars for fullscreen editing

fixing some code from the comments

fbd8a23

fixed the state change and cleaned up code for eslint

0233643

reworking architecture of code

a1f4e32

forgot this in last commit

780712b

cleaning up code and fixing slider bug on fullscreen mode

90c0aab

for got run eslint

90bff90

added fullscreen button for notes and snippets

d1e22e7

allows user to hide the sidebars for fullscreen editing

fixing some code from the comments

5325914

fixed the state change and cleaned up code for eslint

9816393

reworking architecture of code

e2a7172

cleaning up code and fixing slider bug on fullscreen mode

3dbbdc6

for got run eslint

27c1d65

Merge branch 'master' into master

8617442

asmsuechan approved these changes Jun 12, 2017

View reviewed changes

asmsuechan mentioned this pull request Jun 12, 2017

Handling pull 602 #632

Merged

asmsuechan closed this Jun 12, 2017


		handleFullScreenButton (e) {

		this.state.fullScreen.status = !this.state.fullScreen.status

Uh oh!

Conversation

nadr0 commented May 28, 2017

Uh oh!

asmsuechan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmsuechan May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmsuechan May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadr0 commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmsuechan commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmsuechan May 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmsuechan May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nadr0 May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmsuechan May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmsuechan commented Jun 3, 2017

Uh oh!

nadr0 commented Jun 4, 2017

Uh oh!

asmsuechan left a comment •

edited

Loading

asmsuechan May 28, 2017 •

edited

Loading

asmsuechan May 28, 2017 •

edited

Loading

asmsuechan May 29, 2017 •

edited

Loading

asmsuechan May 30, 2017 •

edited

Loading

nadr0 May 31, 2017 •

edited

Loading

asmsuechan May 31, 2017 •

edited

Loading

asmsuechan commented Jun 5, 2017 •

edited

Loading