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

Implement options page to persist Prettier config #56

Merged
merged 40 commits into from Oct 29, 2019
Merged

Conversation

nickmccurdy
Copy link
Member

@nickmccurdy nickmccurdy commented Oct 23, 2019

Fixes #25

I still need to improve the UI, but feedback is welcome

image

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Code mostly looks good to me, but I don't think this needs to be a completely separate app. Can we just update the webpack config to output a second bundle? We don't need to compile anything except JSX, I think, so adding and configuring Babel should be fairly straightforward.

extension/options/src/App.js Outdated Show resolved Hide resolved
extension/options/src/App.js Outdated Show resolved Hide resolved
extension/options/src/App.js Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
extension/options/src/App.js Outdated Show resolved Hide resolved
extension/options/src/App.js Outdated Show resolved Hide resolved
@nickmccurdy
Copy link
Member Author

nickmccurdy commented Oct 23, 2019

I don't think this needs to be a completely separate app. Can we just update the webpack config to output a second bundle? We don't need to compile anything except JSX, I think, so adding and configuring Babel should be fairly straightforward.

I was originally hoping that CRA would make the build setup easier, but it's kind of a hassle to manually run two different build scripts depending on which files change. I'll give this a shot.

@nickmccurdy
Copy link
Member Author

nickmccurdy commented Oct 23, 2019

Travis seems to be stalling again, I don't even see these commits in the PR builds. 😢 Though oddly enough, the branch builds still work fine.
image

@lipis
Copy link
Member

lipis commented Oct 23, 2019

Seems to be OK now.. or?

extension/options/public/index.html Outdated Show resolved Hide resolved
extension/options/src/App.js Outdated Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
@nickmccurdy
Copy link
Member Author

nickmccurdy commented Oct 24, 2019

I implemented a custom webpack config and moved the files around a bit. Do we like this setup? How would we feel about moving the contents of extension to the root and publishing dist instead so there aren't any extra files? Building and installing the extension is much faster without CRA's node_modules in the extension/options directory though.

@lipis
Copy link
Member

lipis commented Oct 26, 2019

let's make it work.. then make it better :)

@kaicataldo
Copy link
Member

I think this is a good foundation for the settings page, and once we change this we could merge this and follow up with styling and moving files around!

@nickmccurdy
Copy link
Member Author

Yea sorry, still working on that. I ran into some issues and set up devtools to make it easier to troubleshoot, but still need to finish tweaking its configuration.

@nickmccurdy
Copy link
Member Author

nickmccurdy commented Oct 29, 2019

Now that #59 is merged (which was an interactive rebase of this branch), should I merge or rebase this on top of master?

@lipis
Copy link
Member

lipis commented Oct 29, 2019

No need to rebase.. we are squashing the PRs anyways.

@nickmccurdy nickmccurdy marked this pull request as ready for review October 29, 2019 16:14
@nickmccurdy
Copy link
Member Author

I figured out how to scope the options property in Chrome's extension storage. I can help out with publishing and UI fixes after this is merged.

@lipis
Copy link
Member

lipis commented Oct 29, 2019

I couldn't find a way to add more admins to the extension dashboard.. or how could I transfer it to another account so we can enable the automatic publishing.

@nickmccurdy
Copy link
Member Author

To be clear, I don't need publishing access for this PR specifically, as the storage is automatically set up with the user's Chrome storage and Google account (if any) due to the storage permission in package.json.

If you want to set up group publishing see Set up Group Publishing. Unfortunately this process is irreversible and charges each new publisher $5, but we could try auto publishing with a single account if you prefer (which I think I would too).

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -482,4 +486,4 @@ function init() {
}
}

init();
chrome.storage.sync.get(init);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessarily a blocker (we can fix this later if need be), but this only runs when the page is loaded, correct? One use case this might not handle very gracefully is when a GitHub/StackOverflow tab is open and someone goes and changes the settings (they wouldn't go into effect until the page is reloaded).

We could probably fix this by reading from chrome storage each time Prettier formats (again, does not need to be a blocker to land this, just want to make sure it's commented here for posterity).

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to implement this, but we should be able to use chrome.storage.onChanged.addListener to subscribe to option updates in the content script.

@kaicataldo kaicataldo merged commit 2491b6a into master Oct 29, 2019
@nickmccurdy nickmccurdy deleted the options branch October 30, 2019 10:23
@lipis
Copy link
Member

lipis commented Oct 31, 2019

Published in 0.0.4

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.

Config support
3 participants