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

Adds functionality to toggle the notebooks and tags on the sidebar. #1002

Merged
merged 4 commits into from Jan 9, 2019

Conversation

Projects
None yet
5 participants
@Abijeet
Copy link
Contributor

Abijeet commented Nov 23, 2018

Towards #222

No work has been done on this part of the request,

On a similar note, I currently have to scroll waaay down my tag list to see the sync status - I feel this should be visible all the time, since it's important info (or optionally, perhaps).

Expanded tag list

joplin-expanded

Collapsed tag list

joplin-collapsed

@Abijeet Abijeet force-pushed the Abijeet:toggle-menu branch 3 times, most recently from 1a3900f to 2b1b862 Nov 23, 2018

@Abijeet Abijeet changed the title WIP: Toggle menu Added functionality to toggle the notebooks and tags. Nov 24, 2018

this.setState({
[toggleKey]: !isExpanded
});
Setting.setValue(toggleKey, !isExpanded);

This comment has been minimized.

@Abijeet

Abijeet Nov 24, 2018

Contributor

I wanted to isolate the changes to this component itself, hence assigning the Setting value here itself. Let me know if that is not appropriate.

This comment has been minimized.

@laurent22

laurent22 Dec 5, 2018

Owner

Yes I think this is fine.

Adds functionality to toggle the notebooks and tags on the sidebar.
Signed-off-by: Abijeet <abijeetpatro@gmail.com>

@Abijeet Abijeet force-pushed the Abijeet:toggle-menu branch from fedcfcd to e78aa22 Nov 24, 2018

@Abijeet Abijeet changed the title Added functionality to toggle the notebooks and tags. Adds functionality to toggle the notebooks and tags on the sidebar. Nov 24, 2018

@Abijeet

This comment has been minimized.

Copy link
Contributor

Abijeet commented Nov 24, 2018

@laurent22 - Ready for review.

@@ -440,10 +446,40 @@ class SideBarComponent extends React.Component {
makeHeader(key, label, iconName, extraProps = {}) {
const style = this.style().header;
const icon = <i style={{ fontSize: style.fontSize * 1.2, marginRight: 5 }} className={"fa " + iconName} />;

if (extraProps.toggleblock || extraProps.onClick) {

This comment has been minimized.

@laurent22

laurent22 Dec 5, 2018

Owner

Please use camelCase (i.e. should be "toggleBlock")

This comment has been minimized.

@Abijeet

Abijeet Dec 13, 2018

Contributor

Hey @laurent22 - This is a custom DOM attribute. I think React does not allow us to camelCase these.

This comment has been minimized.

@laurent22

laurent22 Dec 13, 2018

Owner

Hi @Abijeet, you're correct! Let's keep that way then.

return (
<div style={style} key={key} {...extraProps}>
<div style={style} key={key} {...extraProps} onClick={async (event) => {

This comment has been minimized.

@laurent22

laurent22 Dec 5, 2018

Owner

Please create a class method for the onClick handler in the constructor, like I've done for this.onFolderToggleClick_ and other methods. This is to avoid anonymous functions in components, which can trigger unnecessary renderings.

@laurent22

This comment has been minimized.

Copy link
Owner

laurent22 commented Dec 5, 2018

Thanks @Abijeet, I've just noticed two small issues but otherwise it looks good. Once these two issues are fixed I will merge.

@alexdevero

This comment has been minimized.

Copy link
Contributor

alexdevero commented Dec 10, 2018

Any news about this PR?

@Abijeet

This comment has been minimized.

Copy link
Contributor

Abijeet commented Dec 10, 2018

@alexdevero - I'll check this today or tomorrow.

Modified to not use an anonymous function.
Signed-off-by: abijeetpatro <abijeetpatro@gmail.com>
return (
<div style={style} key={key} {...extraProps}>
<div style={style} key={key} {...extraProps} onClick={async (e) => { await this.onHeaderClick_(headerClick, key, extraProps.toggleblock, e); }}>

This comment has been minimized.

@laurent22

laurent22 Dec 13, 2018

Owner

@Abijeet, thanks for making the change, but I now see it's not that straightforward not to use an anonymous function due to the dependencies to other variables. In that case, would mind reverting to the previous version? The previous code was clearer, and while it's better not to use anonymous functions we can still have one here and there for non-critical code.

This comment has been minimized.

@Abijeet

Abijeet Dec 20, 2018

Contributor

@laurent22 -I've made some modification and I think what I've done now is best of both worlds. Let me know if you want me to put the entire code inline instead.

@Abijeet

This comment has been minimized.

Copy link
Contributor

Abijeet commented Dec 15, 2018

Hey guys, I'm moving out of station in a few days, and quite busy with the packing. I'll need time till next weekend to wrap this up.

@alexdevero

This comment has been minimized.

Copy link
Contributor

alexdevero commented Dec 15, 2018

@Abijeet do you want some help? With code, not packing 🙂.

Updated the code to be cleaner.
Signed-off-by: abijeet <abijeetpatro@gmail.com>
@Abijeet

This comment has been minimized.

Copy link
Contributor

Abijeet commented Dec 20, 2018

@alexdevero - Thanks for the help! :)

Done with the moving, will have time to contribute now.

@alexdevero

This comment has been minimized.

Copy link
Contributor

alexdevero commented Dec 20, 2018

@Abijeet I can't wait to see this live.

@Abijeet

This comment has been minimized.

Copy link
Contributor

Abijeet commented Jan 1, 2019

Hey @laurent22 - Happy new year. I've resolved the conflicts, please review again when you have time.

@laurent22 laurent22 merged commit 8328119 into laurent22:master Jan 9, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@laurent22

This comment has been minimized.

Copy link
Owner

laurent22 commented Jan 9, 2019

That looks good, thanks @Abijeet!

By the way, in here, what was the reason for supporting extraProps.onClick? Is it currently used somewhere?

@AbijeetP

This comment has been minimized.

Copy link

AbijeetP commented Jan 9, 2019

@laurent22 - It wasn't being used anywhere but I didn't want anyone developing a feature later wondering why the heck is their onClick not triggering.

@Abijeet Abijeet deleted the Abijeet:toggle-menu branch Jan 10, 2019

@photonxp

This comment has been minimized.

Copy link

photonxp commented Jan 11, 2019

Left a comment about an alternative solution by means of the "relative height" and "make always visible" to:
#222 (comment)

@photonxp photonxp referenced this pull request Jan 11, 2019

Open

Fold/unfold Notebook/Tag trees + sync status visibility #222

2 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment