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

Refactor status bars #485

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

stefan-toubia
Copy link
Contributor

@stefan-toubia stefan-toubia commented Nov 24, 2019

What has Changed?

  • Refactor and break up status bars
  • Add ConfigReader for managing color config

This PR is only a refactor and is to be followed by the next PR. Please review here.. This PR replaces #480 with a different implementation approach.

While working on adding evaluation status to the app state, I saw a benefit from refactoring the status bars to make things more manageable. I ran into a dependency resolution issue where paredit/statusbar.ts was depending on the colors var from /statusbar.ts, so I also took this opportunity to put down groundwork for a ConfigReader with a purpose of creating one place to manage reading the workspace config.

I think there is room for improvement on the ConfigReader, as it only provides color config right now. I feel it should also be initialized and managed somewhere else in the application, but at least with the way it is right now, it has no other dependencies so should be able to safely reference it anywhere else in the application.

Fixes #

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.) You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. (For now you'll need to opt in to the CircleCI New Experience UI to see the Artifacts tab, because bug.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse

@stefan-toubia stefan-toubia changed the title Stefan/refactor status bars Refactor status bars Nov 24, 2019
@PEZ
Copy link
Collaborator

PEZ commented Nov 24, 2019

This is awesome. Thanks!

I am reading through the changes some and will want to test it a bit, naturally. Saw two things at a quick glance:

  1. The file names break our convention with using kebab-case. (I added that to the Style Guide now.)
  2. The classes for different status bar items should, IMO, be named with Item at the end. E.g: ConnectionStatusBarItem.

@stefan-toubia
Copy link
Contributor Author

@PEZ Ok no problem, I overlooked the file naming and will fix that, and I agree on the class names, will fix those as well.

I added a link to the followup PR so be sure to check that out as well.

Comment on lines +57 to +59
// TODO: This should be somewhere else
const configReader = new ConfigReader();
export default configReader;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that you are improving the config reading. This is just a note to us all to be alert to how this work can be continued. There are a few other config watchers, I think. And also we are reading up most config in the state module, where I do not think it really belongs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and maybe it would be a good idea to consume the config reader from state to keep aligned with the single source of truth. Config reader could emit change events to be picked up by state. Or if using redux, dispatch config change actions. Tomato/Tomato.

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.

2 participants