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 Padding setting #155

Merged
merged 9 commits into from Dec 26, 2018
Merged

Add Padding setting #155

merged 9 commits into from Dec 26, 2018

Conversation

@fauzie811
Copy link
Contributor

@fauzie811 fauzie811 commented Dec 17, 2018

Copy link
Contributor

@ericcornelissen ericcornelissen left a comment

Here's my two cents on your implementation. Also the setting does not seem to be working for me, even when I restart the application 🤔

Margin="0,0,0,24"
HorizontalAlignment="Left"
Header="Padding"
Maximum="100"

This comment has been minimized.

@ericcornelissen

ericcornelissen Dec 17, 2018
Contributor

Although it is very subjective, I don't think anyone would want a padding of 100px. Personally I would set this to something in the range of 30 (perhaps somewhat low) to 50 (already very high).

This comment has been minimized.

@fauzie811

fauzie811 Dec 17, 2018
Author Contributor

I agree. I think 30 is enough.

ericcornelissen and others added 2 commits Dec 17, 2018
Co-Authored-By: fauzie811 <fauzie811@yahoo.com>
@fauzie811
Copy link
Contributor Author

@fauzie811 fauzie811 commented Dec 17, 2018

To be honest. I haven't compiled the code myself, because I couldn't. I haven't finished downloading the Windows 10 SDK. I'm so sorry, I just couldn't wait.

In theory, it should work now. I guess.

@ericcornelissen
Copy link
Contributor

@ericcornelissen ericcornelissen commented Dec 17, 2018

Haha okay, no problem (perhaps say that in the PR description next time 😉). I also figured out why it wasn't working for me, simply because I didn't rebuild the script 😅

The problem I'm having right now is that when the padding is set to a value higher then 12px it pushes the .terminal outside the limits of the container. I imagine this can be fixed by using box-sizing but it didn't work upon my first and only try 😞

fauzie811 added 2 commits Dec 17, 2018
Add "px" suffix to padding slider tooltip
@fauzie811
Copy link
Contributor Author

@fauzie811 fauzie811 commented Dec 17, 2018

This time I'm 100% sure it'll work. 🤞

Maybe. 🤣

Copy link
Contributor

@ericcornelissen ericcornelissen left a comment

Can confirm it works now 👍 Good job on the implementation @fauzie811 I've got just one more comment

After that its up to @felixse to decide on the padding value range and merge the PR 😃

FluentTerminal.Client/src/index.js Outdated Show resolved Hide resolved
@ericcornelissen
Copy link
Contributor

@ericcornelissen ericcornelissen commented Dec 17, 2018

I will just leave this preview I made here for anyone interested. Note that the visual bugs regarding the text is caused by xterm's term.fit().

padding_preview

@felixse
Copy link
Owner

@felixse felixse commented Dec 18, 2018

Looks great @fauzie811 👍
And also thanks for reviewing this @ericcornelissen

After that its up to @felixse to decide on the padding value range and merge the PR 😃

Lets make it 64px 🤔

@felixse felixse added this to the 0.2.2.0 milestone Dec 26, 2018
@felixse felixse merged commit 9bf8284 into felixse:master Dec 26, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants