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

CIO-312 Remove similar block #318

Merged
merged 1 commit into from
Feb 16, 2019

Conversation

sog01
Copy link
Contributor

@sog01 sog01 commented Feb 15, 2019

Fixes #312

Isolate common style

Description

Isolate common style at 3 exported const which are statusBarContainer, statusBar and messageCenter

Motivation and Context

To dry up code, Fixes #312.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@sog01 sog01 requested a review from jwu910 as a code owner February 15, 2019 01:02
@joshuabot
Copy link

joshuabot commented Feb 15, 2019

Warnings
⚠️

Changes to app files were detected, did you forget a CHANGELOG entry? You can find it at CHANGELOG.md

Generated by 🚫 dangerJS

@jwu910 jwu910 self-assigned this Feb 15, 2019
@sog01
Copy link
Contributor Author

sog01 commented Feb 15, 2019

Please review @jwu910, can I just remove props on similar block ?

src/utils/interface.js Outdated Show resolved Hide resolved
@sog01 sog01 force-pushed the CIO-312-remove-similar-block branch from f8858c5 to b26c9e0 Compare February 15, 2019 02:40
@jwu910
Copy link
Owner

jwu910 commented Feb 15, 2019

Just started reviewing :)

:octocat: Sent from GH.

src/utils/interface.js Show resolved Hide resolved
width: '100%',
});

const messageCenter = blessed.log(themeCommon(2, 5));
Copy link
Owner

Choose a reason for hiding this comment

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

With the comment above we can keep this as

const blessedObject = blessed.element({
	...baseTheme,
	bottom: 2,
	height: 5,
});

Let me know what you think! Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I though that bottom and height also noticed as a similar, so I localize using a function which passing bottom and height as a params. Since they have different value.

That should be simply if only using an object. Thanks for suggestion @jwu910 !. I'll fix then.

Copy link
Owner

Choose a reason for hiding this comment

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

That makes sense! Don't worry, I just think that the flow of understanding the code for future maintainability may be more straight forward if we just use a plain object for now.

@jwu910
Copy link
Owner

jwu910 commented Feb 15, 2019

By the way @sog01 my development branch was just updated, be sure to get the changes!

@sog01
Copy link
Contributor Author

sog01 commented Feb 15, 2019

Sure @jwu910, I'll be careful

@sog01 sog01 force-pushed the CIO-312-remove-similar-block branch 2 times, most recently from 4061f16 to 6d32101 Compare February 15, 2019 07:27
@sog01
Copy link
Contributor Author

sog01 commented Feb 15, 2019

Please check again @jwu910

@jwu910
Copy link
Owner

jwu910 commented Feb 15, 2019

Just started reviewing :)

:octocat: Sent from GH.

Copy link
Owner

@jwu910 jwu910 left a comment

Choose a reason for hiding this comment

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

lgtm

@jwu910
Copy link
Owner

jwu910 commented Feb 15, 2019

@sog01 One last thing,

Can you add a changelog entry for your fix?

@sog01
Copy link
Contributor Author

sog01 commented Feb 15, 2019

@jwu910 sure, may I also change date into today ? Since this changed happen right now.

@jwu910
Copy link
Owner

jwu910 commented Feb 15, 2019

@sog01 You don't need to include a date on the changelog entry. Just the summary of work, you can place it under the "Unreleased" header. You can look at the other changelog entires for reference.

@sog01 sog01 force-pushed the CIO-312-remove-similar-block branch from 6d32101 to 183b581 Compare February 15, 2019 09:32
@sog01
Copy link
Contributor Author

sog01 commented Feb 15, 2019

Okay please check again @jwu910

@jwu910
Copy link
Owner

jwu910 commented Feb 15, 2019

@sog01 Your entry in the Changelog should fall under Unreleased because this issue is not yet released.

@sog01 sog01 force-pushed the CIO-312-remove-similar-block branch from 183b581 to ac72b37 Compare February 15, 2019 10:02
@sog01 sog01 force-pushed the CIO-312-remove-similar-block branch from ac72b37 to fb15847 Compare February 15, 2019 10:03
@sog01
Copy link
Contributor Author

sog01 commented Feb 15, 2019

Sorry my bad @jwu910 🙏, It should be okay right now :)

@jwu910 jwu910 merged commit a2d47b5 into jwu910:development Feb 16, 2019
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.

3 participants