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 multi-page UI for settings & imports #30

Merged
merged 9 commits into from
Feb 20, 2017
Merged

Conversation

aquibm
Copy link
Collaborator

@aquibm aquibm commented Feb 19, 2017

Overview

Adds functionality to easily create multiple pages within the options of the chrome extension. We do this by creating a separate react application with hash-based routing. The feature is triggered by clicking the 'Options' link in chrome://extensions

Demonstration

memex-options

Future considerations

  • Split out common libraries (react, redux, etc) used by the overview and options applications into a common vendor.js to improve performance.
  • Add a link to access the options from the overview app.
  • Add a link to access the overview from the options app.

If accepted, this PR would fix #19

@Treora
Copy link
Collaborator

Treora commented Feb 19, 2017

Awesome, this looks great. Nice design as well.

As usual, there are a few things in the code that I would like to either modify or understand the reason behind:

  • Why are most of styles defined in javascript instead of the css file? I would simply give each component a className equal to the component name (like the overview), and style that class with css. Perhaps it would be nicer to have the style in the same folder as the component it styles, but for that we could start using Compass/Sass/SCSS at some moment.
  • I suppose PageTitle need not be a separate component if the styles are reorganised such a way.
  • I wondered if the routes and navigation entries could be defined in a single place, as they correspond one-to-one. Not too important though, only do this if this does not overcomplicate things.
  • Some components are classes, some are functions, in what looks like an arbitrary inconsistency. Either way is fine, I guess I would use functions as they only have a render method.
  • Should Layout.js logically be in containers/? I may not have properly understood the division between container components and other components though; the boundary seems blurry to me so I usually just make a folder with components. Do you think that would be appropriate or messy?
  • Style consistency nitpicking: some files end without a newline, and semicolons are so ES5.

@@ -3,4 +3,4 @@ dist
extension/background.js
extension/content_script.js
extension/overview/overview.js

extension/options/options.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you rebase -i the commits and fixup this commit to glue it into the one before it? Otherwise this repo will keep carrying 2MB of unused code around for ever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey, I've run git filter-branch to remove all traces of options.js from the commit history. Commit 50a2ffB no longer includes the file.

@aquibm
Copy link
Collaborator Author

aquibm commented Feb 19, 2017

Why are most of styles defined in javascript instead of the css file?

This is the easiest approach I've come across to bundle together everything that's required for a component to render. The preferred option in my opinion would be to use css-modules but that requires a lot of setup and could be a future enhancement.

I suppose PageTitle need not be a separate component if the styles are reorganised such a way.

Yeah, I guess I got a bit too carried away with abstracting out components that can be reused. I'll fix this.

I wondered if the routes and navigation entries could be defined in a single place, as they correspond one-to-one.

That's a really good point, I'll take a brief look into it.

Some components are classes, some are functions, in what looks like an arbitrary inconsistency.

In my view, container components should be where all of the event handling and simple application logic is contained. Components should just be "dumb" helpers that take in props and render render them. This is why I've used class notation for containers where I'm expecting event handlers and other functionality to be added whereas navigation and other components are simple functions.

Style consistency nitpicking: some files end without a newline, and semicolons are so ES5.

I'll fix the newlines, but comments are ingrained in me I'm afraid 😛

Todo list:

  • Remove PageTitle component
  • Define routes in a single place
  • Fix newlines at end of files

@blackforestboi
Copy link
Collaborator

Awesome, this looks great. Nice design as well.

Agreed, this looks awesome!

@Treora
Copy link
Collaborator

Treora commented Feb 20, 2017

Ok, let's keep the style in javascript for now and set up sass or css-modules soon.

Components should just be "dumb" helpers that take in props and render render them. This is why I've used class notation for containers where I'm expecting event handlers and other functionality to be added

Understood. My expectation is just a bit different, as I tend to use Redux for all state management, making that pretty much every defined component is a dumb prop-renderer (where event handlers are props too).

Let's move on though, it's fine as it is. I'll just do a quick sed -i s/;$// to drop the semicolons and then merge it.

One question still: I would like to publish the whole thing in the public domain, free from copyright restrictions (see e.g. Unlicense or CC0). Are you okay with a "relinquishment in perpetuity of all present and future rights to this software under copyright law"?

@aquibm
Copy link
Collaborator Author

aquibm commented Feb 20, 2017

@Treora I'm okay with that. We should also look into bringing in eslint to automatically complain about semicolons and other undesirables at build time.

@Treora Treora merged commit 805f644 into WebMemex:master Feb 20, 2017
@Treora
Copy link
Collaborator

Treora commented Feb 20, 2017

So, first PR merged. 🎉
Thanks Aquib!

@aquibm
Copy link
Collaborator Author

aquibm commented Feb 20, 2017

Awesome, onto the next one 👍

@blackforestboi
Copy link
Collaborator

Wohoo! Off to new worlds!

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.

Multi-page UI with react-router?
3 participants