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

Redesign the options page #54

Merged
merged 26 commits into from Jul 18, 2016
Merged

Conversation

@iamakulov
Copy link
Collaborator

iamakulov commented Feb 11, 2016

I’ve cleaned up the extension options page slightly and improved accessibility. Before and after:

The options page before the cleanup and after it


The list of changes:

26b7f33 – Replace spans with labels for better accessibility
This makes focusing fields and checking checkboxes easier. When you add a label and link it with an input, this label becomes clickable, and you can click it to check the checkbox or focus the input it’s linked with.

186f087 – Implement proper wrapping
In forms, line breaks are usually created with <br/>. This commit removes the .caption width limitation to make wide captions look properly and implements proper line breaks instead.

9300cbd – Resize checkboxes and align them
A large checkbox is blurry, a smaller one looks way better. Also, baseline alignment is used instead of the default middle one because it also looks better.

c2bdbd2 – Swap labels and checkboxes
Make the labels appear after the checkboxes to follow common UI patterns. Also, include the captions into the labels to increase the clickable area.

861e1f7 – Make text input non-bold
The bold text looks like a design exception and seem to have no meaning. This in fact is just a guess, so please correct me if I’m wrong.

Looking forward to your comments.

iamakulov added 5 commits Feb 11, 2016
This makes focusing fields and checking checkboxes easier. The `label` element is suited to be used specially with inputs. When you add a label and link it with an input, this label becomes clickable, and you can click it to check the checkbox or focus the input it’s linked with.
In forms, line breaks are usually created with <br/>. This commit removes the .caption width limitation to make wide captions look properly and implements proper line breaks instead.
A large checkbox is blurry, a smaller one looks way better. Also, `baseline` alignment is used instead of the default `middle` one because it also looks better.
Make the labels appear after the checkboxes to follow common UI patterns. Also, include captions into labels to increase the clickable area
The bold text looks like a design exception and seem to have no meaning
@iamakulov
Copy link
Collaborator Author

iamakulov commented Feb 11, 2016

In fact, the options page requires a wider redesign from the UX point due to the several issues. It isn’t critical, but it’s a positive change that could save each user who tries to configure something several minutes of time. For example, currently it’s unclear that blacklisting/whitelisting of actions will be applied only when the corresponding checkbox is checked, or that “Page urls to inject DevTools in” will be used when the “Inject in all pages” checkbox is unchecked. It is written, but it isn’t recognizable from the first sight. A quick-and-dirty solution could be to disable the text boxes when the data from them is not used. A better one involves implementing switches like “Inject into: 🔘 all pages; 🔘 specific pages: ________”.

I’ve tried fixing these issues in part of this PR, but the amount of work is quite large. The options page should support radio buttons and should re-render itself when something changes to make applying conditional classes possible. I don’t know if I manage to do this in the near time (most likely no), so I’m just leaving this note here to document (or something) these problems.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 12, 2016

Hey @iamakulov,

Thanks a lot for this contribution! I guess those styles problems appear on Windows (due to using other fonts by Chrome or because the scrollbar takes some width), right? I didn't get to test there. Just so you know it looked on El Capitan (where Chrome uses Helvetica Neue by default) like that:

screen

What do you think of making it more compact as above?

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 12, 2016

About checkboxes for Filter spec. actions and Inject in all pages totally agree. We should move everything to a React component (instead of rendering directly), add states and hide inactive form elements. It would be cleaner and the form would be compact.

@iamakulov
Copy link
Collaborator Author

iamakulov commented Feb 12, 2016

What do you think of making it more compact as above?

Wow, this form looks way better. I still doubt about the checkbox placement (the common UI pattern is checkbox + label, not label + checkbox), but totally agree that this layout is more suitable.

Yep, let’s try moving options to a separate component and fixing the issues. Will return to this in a week.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 12, 2016

Ok. We can move checkboxes to the left side and, as we have enough space there, replace them with switchers.

Instead of adjusting width manually as I did before, se should better use flexbox style with flex-wrap: nowrap for every line containing 2 elements.

@iamakulov
Copy link
Collaborator Author

iamakulov commented Feb 12, 2016

I see. Will use switchers and flexbox then.

@iamakulov iamakulov changed the title Clean up the options page [WIP] Clean up the options page Feb 18, 2016
iamakulov added 6 commits Feb 18, 2016
The layout was rewritten in flexbox to make selects and textareas grow for full width.

.option_type_textarea .option__caption cannot be baseline-aligned with `align-items: baseline` due to a Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=588482. Therefore, a top margin is used. The margin value might be non-portable to other browsers and OSes and should be checked.

 .option_type_select .option__caption also cannot be baseline-aligned with `align-items: baseline`. Instead, the caption is assigned the line-height which equals to to the select height. This might also be non-portable.
index.js:42: syncOptions.get() receives an anonymous wrapping callback instead of `renderOptions`. This is done to made differences between its behavior and `.subscribe()` behavior cleaner. `.get()` calls a callback in a Node style, whereas `.subscribe()` saves the callback and doesn’t call it in the Node style.

A new `subscribe()` method is added to syncOptions. It receives a callback or an array of callbacks and calls them each time the options are updated.
@iamakulov
Copy link
Collaborator Author

iamakulov commented Feb 23, 2016

@zalmoxisus, a couple of questions while I’m redesigning the options:

  • Why is the autocommit option a thing? Why are actions automatically committed when they reach like 50 items?
  • What does the State serialization option do?
@gaearon
Copy link

gaearon commented Feb 23, 2016

@iamakulov

Just wanted to drop by and thank you for working on this. Most developers including me have troubles figuring out what exactly is off about the UI, and help from people like you is dearly appreciated.

@iamakulov
Copy link
Collaborator Author

iamakulov commented Feb 23, 2016

@gaearon, wow, absolutely unexpected! Thank you for kind words.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 24, 2016

Why is the autocommit option a thing? Why are actions automatically committed when they reach like 50 items?

In most of cases we need only the last changes not thousands of logs. It's intended to solve the performance problems for rendering the monitors and especially for the extension lifecycle.

Chrome doesn't allow the extension to communicate with the app's page directly (though it is declared as being for security reason, it's mostly imposed by design as the page and the extension run in different processes), that's why we relay the changes to the extension's content script, which sends them to the extension's background script, from there the extension's window can use them. This is an expensive operation especially when serializing the states. Though we made a lot of optimizations (now we mostly don't pass the full lifted state, but only the changes) it is still the bottleneck for productivity, and this object shouldn't grow infinitely.

What does the State serialization option do?

We need this only when using Immutable.js or having circular references in states. In these cases the states and action's payload objects will be serialized via jsan, so we don't lose any data by passing to the extension background script. In other cases we have plain objects and can gain some performance by disabling this option.

When we get #60 fixed, we would have more user cases for serialization, but I'm not sure we should nuke this option.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 24, 2016

@gaearon, exactly, thank you all for all the help here!

@iamakulov
Copy link
Collaborator Author

iamakulov commented Feb 25, 2016

Related: #61.

@iamakulov
Copy link
Collaborator Author

iamakulov commented Feb 27, 2016

Currently busy, will return to this in three-four days. (Just letting you know that I haven’t stopped working here.)

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 27, 2016

@iamakulov, take your time. When you need to review something you did, let me know.

@iamakulov iamakulov force-pushed the iamakulov:update-options-page branch from eab09f1 to 3bc1204 Mar 22, 2016

/* Checkbox styling */
.option_type_checkbox .option__element {
/* Radio button in Chrome is 2px narrower than the checkbox.

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 10, 2016

Author Collaborator

A doubtful moment. Here I assume that the only browser we support is Chrome. I should’ve probably try to build and install the Firefox extension and check how the checkboxes look there, but I haven’t found any documentation on this so I went to the next task. Should we test this in Firefox?

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Jul 11, 2016

Owner

No. We'll do it as a separate PR when we'll be ready for that. It will look different in FF for sure, as we're including the Chrome css file here.

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 11, 2016

Author Collaborator

BTW, the comment is outdated, will update it today. (Adding #todo for this discussion to search later.)

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 17, 2016

Author Collaborator

Done.

@iamakulov
Copy link
Collaborator Author

iamakulov commented Jul 10, 2016

Apart from those comments, there’re some more general ones:

  • We had a stub function for showing errors when a user enters an invalid regexp, but it didn’t do anything, and I removed it. In fact, we need to show an error in such cases, but let’s do this as a separate task so that this PR isn’t blocked.
  • You’ve told that the “Serialize state” option should be a select box, but as far as I understand the new serialization values aren’t implemented yet so I didn’t design and implement the switcher for them. Am I right?
  • It’d be great to write the option descriptions for the “More info” links in Wiki (as I mentioned in #54 (comment)). Can we do this? I can help with writing texts. (I think we’re better to do this as a separate task to not block this PR.)
@zalmoxisus
Copy link
Owner

zalmoxisus commented Jul 11, 2016

Looks amazing. Thanks a lot for implementing this! i'll respond inline.

We had a stub function for showing errors when a user enters an invalid regexp, but it didn’t do anything, and I removed it. In fact, we need to show an error in such cases, but let’s do this as a separate task so that this PR isn’t blocked.

OK.

You’ve told that the “Serialize state” option should be a select box, but as far as I understand the new serialization values aren’t implemented yet so I didn’t design and implement the switcher for them. Am I right?

We can start it from here as it would be a part of migrateOldOptions function. Having 1 and 2 instead of true will not break the current implementation. I will implement the last option after this PR, as it's just commented there.

It’d be great to write the option descriptions for the “More info” links in Wiki (as I mentioned in #54 (comment)). Can we do this? I can help with writing texts. (I think we’re better to do this as a separate task to not block this PR.)

Agree about making it in a separate PR. Maybe better to add it in docs, so we could later move it to GitBook?

{' '}
<label className="option__label" htmlFor="maxAge">items</label>
<div className="option__hint">
When the number is reached, DevTools starts removing the oldest actions. Improves the DevTools performance.

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 11, 2016

Author Collaborator

Not sure about plurality here. What sounds better to you: “DevTools starts” or “DevTools start”?

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Jul 11, 2016

Owner

Maybe When the number is reached, the oldest actions are getting removed. :-)

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 11, 2016

Author Collaborator

I usually prefer active voice over the passive one because it’s more explicit, and people perceive it easier. So I’d better stick with DevTools start(s). :–)

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Jul 11, 2016

Owner

It would be confusing to indicate that it could be one or more. Actually, redux-devtools-instrument is removing, so better to use "the instrumentation enhancer starts removing", or just "the enhancer". It will also indicate that it is referred only to the case when using the extension as a Redux enhancer. As the extension can be used now not only for Redux.

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 12, 2016

Author Collaborator

It would be confusing to indicate that it could be one or more

Huh, I’ve actually meant not that we should write exactly “DevTools start(s)”, but that we should write either “DevTools start” or “DevTools starts” :–)

As to “enhancer” instead of “DevTools”, I wouldn’t do this. “Enhancer” is more complex than “DevTools” because it’s an implementation detail, and the users may not know it. (Hope I explained this well.)

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 12, 2016

Author Collaborator

Will change this to “DevTools start”.

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 17, 2016

Author Collaborator

Done.

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 17, 2016

Author Collaborator

Done.

<label className="option__label" htmlFor="maxAge">items</label>
<div className="option__hint">
When the number is reached, DevTools starts removing the oldest actions. Improves the DevTools performance.
{' '}<a href="https://github.com/zalmoxisus/redux-devtools-extension/pull/54#issuecomment-188167725">More info</a>

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 11, 2016

Author Collaborator

It’s probably better to write this in two lines:

{' '}
<a href="https://github.com/zalmoxisus/redux-devtools-extension/pull/54#issuecomment-188167725">More info</a>

Will do this in the evening.

#todo

This comment has been minimized.

Copy link
@iamakulov

iamakulov Jul 17, 2016

Author Collaborator

Done.

iamakulov added 4 commits Jul 12, 2016
@iamakulov
Copy link
Collaborator Author

iamakulov commented Jul 12, 2016

We can start it from here as it would be a part of migrateOldOptions function.

What if I do this in a separate PR too? This way, we can release the new options dialog earlier. What do you think?

Agree about making it in a separate PR. Maybe better to add it in docs, so we could later move it to GitBook?

Sure! Then my duty is to write and submit the text. #todo

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jul 12, 2016

What if I do this in a separate PR too?

OK, we can do it after this PR.

@iamakulov iamakulov force-pushed the iamakulov:update-options-page branch from 9a397fd to ba61458 Jul 17, 2016
@iamakulov
Copy link
Collaborator Author

iamakulov commented Jul 17, 2016

@zalmoxisus, I think I’m done here. Is there something we should yet do before merging?

The Travis build is failed for some reason, however, both AppVeyor and my local npm run test are OK.

@iamakulov iamakulov changed the title [WIP] Clean up the options page Redesign the options page Jul 17, 2016
@zalmoxisus
Copy link
Owner

zalmoxisus commented Jul 18, 2016

@iamakulov, thanks a lot for all the changes! It looks good to me. The only problem I see is that if Limit the action history value is less than 2, it will broke instrumentation. The user will get an error in the console DevTools.instrument({ maxAge }) option, if specified, may not be less than 2., which is not obvious from where it comes. The notification you added that it should greater or equal to 2 could help. I think better to forbid setting it to a lower value, though we can do it later.

BTW, it looks great also on Firefox:
screen shot 2016-07-18 at 10 55 59 am

@zalmoxisus zalmoxisus merged commit 80a1309 into zalmoxisus:master Jul 18, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iamakulov iamakulov deleted the iamakulov:update-options-page branch Jul 19, 2016
@iamakulov
Copy link
Collaborator Author

iamakulov commented Jul 19, 2016

The notification you added that it should greater or equal to 2 could help. I think better to forbid setting it to a lower value, though we can do it later.

Hm, do I properly understand that you suggest to add a text note like “Can’t be less than 2”?
As to the limit, the value is already limited to 2.

BTW, it looks great also on Firefox:

Hm, I dislike the font, and some items are shifted a bit. I think we can do it better.
You’ve told something about providing a Firefox-specific css file. Can I ask you to create it in the place you find appropriate (could be in a separate branch)? I’ll submit a PR with the changes I believe are necessary.

As to other PRs I’ve promised to make, I’m going to submit them soon.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jul 20, 2016

Hm, do I properly understand that you suggest to add a text note like “Can’t be less than 2”?
As to the limit, the value is already limited to 2.

I tried to set 1 and it works, though it is breaking the user's application. So we shouldn't allow setting this value. We can also make the input's border red to show that there's an error.

Hm, I dislike the font, and some items are shifted a bit. I think we can do it better.
You’ve told something about providing a Firefox-specific css file. Can I ask you to create it in the place you find appropriate (could be in a separate branch)? I’ll submit a PR with the changes I believe are necessary.

Chrome is injecting its css file, but firefox not, unfortunately. To deploy it, you need Firefox Nightly or Developer Edition (as Firefox 48 is not released yet). Then just run npm run build:firefox and load the extension's folder ./build/firefox. After that yo can just as usually click Settings button from our popup (page action page) to open the options page.

zalmoxisus added a commit that referenced this pull request Jul 25, 2016
@zalmoxisus
Copy link
Owner

zalmoxisus commented Jul 25, 2016

Hey @iamakulov,

Just to let you know that I've implemented deep serialization in 8754f4a. There's no need for migrateOldOptions, true/false works as well as 0/1/2.

@iamakulov
Copy link
Collaborator Author

iamakulov commented Jul 26, 2016

@zalmoxisus, cool, thanks for the notification!

@born2net
Copy link

born2net commented Jan 11, 2017

I am using the plugin with a large data set (about 700 items) and when I hit about 1000, things start really slowing down in the application. If I disable the plugin I can go to 20K+ with no performance lag.
I have already disabled the history (down to 5).
I am using it with ngrx not sure if this could be the cause?

tx as always,

Sean.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 12, 2017

@born2net, these options are used by Redux only. For ngrx you have to reimplement them from ngrx/store-devtools. Or you could just pass it as parameter (as you noted already) instead of using the global variable. Another good feature we have here, is that you can pause the recording, so there will be no perf impact at all, but again it should be implemented in ngrx/store-devtools.

@born2net
Copy link

born2net commented Jan 12, 2017

thank you, I was able to "fix" / limit the issue using the maxAge

StoreDevtoolsModule.instrumentStore({maxAge: 2}),

Angular 2 Kitchen sink: http://ng2.javascriptninja.io
and source@ https://github.com/born2net/Angular-kitchen-sink
Regards,

Sean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.