Skip to content

Conversation

@callumbooth
Copy link
Contributor

@callumbooth callumbooth commented Mar 19, 2019

Description

Adds a button to change the orientation of the the split editing window to allow either vertical or horizontal

Screenshot 2019-03-19 at 20 55 31

Screenshot 2019-03-19 at 20 55 38

Issue fixed

#2903

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • ⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
  • 🔘 Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

IssueHunt Summary

IssueHunt has been backed by the following sponsors. Become a sponsor

@MiloTodt
Copy link
Contributor

Could perhaps be better implemented by an option in preferences? I'm not sure if it's something people would switch between frequently enough to need a dedicated button on the UI, and the logo for it is very close visually to the one next to it.

@MiloTodt
Copy link
Contributor

This doesn't appear to work correctly when used alongside toggling the code editor from split to full screen.

Results in unexpected behavior counter to button purpose.

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Mar 20, 2019
@callumbooth
Copy link
Contributor Author

Could perhaps be better implemented by an option in preferences? I'm not sure if it's something people would switch between frequently enough to need a dedicated button on the UI, and the logo for it is very close visually to the one next to it.

I think having to navigate to a new screen and find the orientation setting to change it each time would be quite jarring for the user. I know VSCode has the layout options in "View" of the top menu bar.

location

I agree changing the icon to make it more unique and identifiable would be better.

This doesn't appear to work correctly when used alongside toggling the code editor from split to full screen.

I'm struggling to replicate this. Can you let me know more details and I can investigate further

if (!(editorStyle.fontSize > 0 && editorStyle.fontSize < 132)) editorIndentSize = 4
editorStyle.indentSize = editorIndentSize

if (isStacking) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. I think this would be more readable if you add it like following:

editorStyle = isStacking ? 
  { width: '100%' , height: `${this.state.codeEditorHeightInPercent}%` } :
  { width: `${this.state.codeEditorWidthInPercent}%`, height: '100%' }

previewStyle = isStacking ? 
  { width: '100%', height: `${100 - this.state.codeEditorHeightInPercent}%` } :
  { width: `${100 - this.state.codeEditorWidthInPercent}%`, height: '100%' }

sliderStyle = isStacking ? 
 { left: 0, top: `${this.state.codeEditorHeightInPercent}%` } : 
 { left: `${this.state.codeEditorWidthInPercent}%`, top: 0 }

if/else would be also OK but I would avoid adding each prop line by line & use backtick strings to add the % to the style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I have changed the if statement however, this would override the previously set properties, so I have used object.assign to set the new properties. It might be beneficial to update the babel config to transpile the spread operator.

@AWolf81
Copy link
Contributor

AWolf81 commented May 26, 2019

@callumbooth can you please merge master into your branch?

For me it's also hard to reproduce the toggling issue that @MiloTodt mentioned. A step-by-step guide would help here as it's pretty fast in the screenrecording.

To the icon
I like how you've added the layout toggle button and it looks OK. But I also think adding it to the view menu would be better as that's a rarely used feature - so it shouldn't be in the UI.
@ZeroX-DG what would you prefer? The menu item or the button?

@callumbooth
Copy link
Contributor Author

callumbooth commented Jun 10, 2019

@callumbooth can you please merge master into your branch?

This has been done

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Sorry for the wait 😭 Can you resolve the conflict please.

delfaultStatus: 'PREVIEW', // 'PREVIEW', 'CODE'
scrollPastEnd: false,
type: 'SPLIT', // 'SPLIT', 'EDITOR_PREVIEW'
isStacking: false,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be in UI instead of editor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<img styleName='iconInfo' src={isStacking ? '../resources/icon/icon-panel-split-vertical.svg' : '../resources/icon/icon-panel-split-horizontal.svg'} />
<span lang={i18n.locale} styleName='tooltip'>{
isStacking ? i18n.__('Split Panels Horizontally') : i18n.__('Split Panels Vertically')

Copy link
Member

Choose a reason for hiding this comment

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

Can you adjust the code format here? I think you should separate the src for image and the content for span tag into smaller variables for better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have refactored the image src and the span text into smaller vars.

width: width,
height: height
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the format here too please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Sep 27, 2019
@Flexo013
Copy link
Contributor

Flexo013 commented Oct 8, 2019

To the icon
I like how you've added the layout toggle button and it looks OK. But I also think adding it to the view menu would be better as that's a rarely used feature - so it shouldn't be in the UI.
@ZeroX-DG what would you prefer? The menu item or the button?

Just adding my two cents: I think that adding this to the View menu would also be better. There would be less clutter in the UI, and this feature 100% pertains to the View.

@Flexo013 Flexo013 added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Oct 9, 2019
@ZeroX-DG
Copy link
Member

@callumbooth can you resolve the conflict before I can approve this please? thank you 😃

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Jan 31, 2020

Also I agree with the idea of moving this to the View menu instead of having it in the UI, this will keep the UI cleaner. Can you move this to the View menu?

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Apr 5, 2020

@callumbooth ping

@callumbooth
Copy link
Contributor Author

@ZeroX-DG I've fixed the merge conflicts and moved the button the View menu

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

@callumbooth can you update your code before I approve it?

isLocked: false,
editorType: props.config.editor.type,
switchPreview: props.config.editor.switchPreview,
RTL: false
Copy link
Member

Choose a reason for hiding this comment

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

Please don't remove this RTL key, this is for Right To Left feature, people who use this feature will be very angry 😄

@callumbooth
Copy link
Contributor Author

@ZeroX-DG sorry, thats been fixed

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. labels Apr 23, 2020
@Rokt33r Rokt33r added this to the v0.16.0 milestone Apr 23, 2020
@Rokt33r Rokt33r merged commit 4b67026 into BoostIO:master Apr 23, 2020
@callumbooth callumbooth deleted the fix-2903 branch April 24, 2020 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved 👍 Pull request has been approved by sufficient reviewers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants