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.

Sign up for GitHub

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

Jump to bottom

Notebook Properties Dialog clean Branch for review #2450

Closed
wants to merge 10 commits into from
Closed

Notebook Properties Dialog clean Branch for review #2450

wants to merge 10 commits into from

Conversation

bedwardly-down
Copy link
Contributor

@bedwardly-down bedwardly-down commented Feb 6, 2020 •
edited
Loading

Here's a cleaned up NotebookPropertiesDialog branch.

@bedwardly-down bedwardly-down changed the title Npd redux clean Branch Notebook Properties Dialog clean Branch for review Feb 6, 2020
@bedwardly-down
Copy link
Contributor Author

@laurent22, we have life. The linter was still acting up but here you are.

@tessus tessus added desktop All desktop platforms reviewed-todo labels Feb 6, 2020
laurent22
laurent22 reviewed Feb 7, 2020
View reviewed changes
.eslintignore Outdated Show resolved Hide resolved
@laurent22
Copy link
Owner

Please provide a screenshot of the dialog you've created.

@laurent22
Copy link
Owner

And a screenshot of the app with the icons in the sidebar please.

@bedwardly-down
Copy link
Contributor Author

Before I make most of these corrections, i need some information. I've been using much of the current code as references for the styling and it is obvious that isn't the correct way to handle it. Due to both NotePropertiesDialog and ShareNoteDialog having inconsistent stylings and large chunks of the prior being custom styled, can I please get a style reference for what you are wanting? How set in stone are the styles in theme.js?

@laurent22
Copy link
Owner

How set in stone are the styles in theme.js?

It is set in stone.

Styling across the app is relatively inconsistent but it's been getting better over the past versions. For example before there was many styles for buttons, but now I've converted many to use theme.button. Same for dialog boxes which had various styles, but recently I've created theme.dialogModalLayer and DialogButtonRow, which you've correctly picked up.

So it's more of an incremental effort. For that particular pull request, I've checked again and for now let's leave it as it is but it will have to be made more consistent later.

@bedwardly-down
Copy link
Contributor Author

How set in stone are the styles in theme.js?

It is set in stone.

Styling across the app is relatively inconsistent but it's been getting better over the past versions. For example before there was many styles for buttons, but now I've converted many to use theme.button. Same for dialog boxes which had various styles, but recently I've created theme.dialogModalLayer and DialogButtonRow, which you've correctly picked up.

So it's more of an incremental effort. For that particular pull request, I've checked again and for now let's leave it as it is but it will have to be made more consistent later.

Thanks for getting back to me on this. I can see the whole inconsistent styling be super infuriating. It definitely is for me and could possibly lead to others having the same problem.

@bedwardly-down
Copy link
Contributor Author

bedwardly-down commented Feb 9, 2020 •
edited
Loading

Not perfect yet, but here are the screenshots you asked for, @laurent22. I haven't touched the styling much yet outside of just basic renaming and condensing to one style but have a better understanding of it after fully studying the theme.js file during work.

2020-02-08-193155_1920x1080_scrot
2020-02-08-193204_1920x1080_scrot
2020-02-08-193223_1920x1080_scrot
2020-02-08-193235_1920x1080_scrot
2020-02-08-193241_1920x1080_scrot
2020-02-08-200243_1920x1080_scrot

@bedwardly-down
Copy link
Contributor Author

bedwardly-down commented Feb 9, 2020 •
edited
Loading

App.js would need to be changed in several locations for this bug to be functional. Due to that being outside of the scope of the current project and I haven't studied it enough yet, I'm leaving parts of the MainScreen code as Notebook for the time being until further notice.

laurent22
laurent22 reviewed Feb 11, 2020
View reviewed changes
ElectronClient/app/gui/MainScreen.jsx
Comment on lines -108 to +121
bridge().showErrorMessageBox(_('Please create a notebook first.'));
bridge().showErrorMessageBox(_('Please create a folder first.'));
} else {
await createNewNote(null, false);
}
} else if (command.name === 'newTodo') {
if (!this.props.folders.length) {
bridge().showErrorMessageBox(_('Please create a notebook first'));
bridge().showErrorMessageBox(_('Please create a folder first'));
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry just to be clear: internally, in code, we use the term "folder" everywhere. In strings visible by the user however we use "Notebook", so please revert these two changes.

laurent22
laurent22 reviewed Feb 11, 2020
View reviewed changes
ReactNativeClient/lib/joplin-database.js
@@ -665,6 +665,10 @@ class JoplinDatabase extends Database {
queries.push(this.addMigrationFile(27));
}

if (targetVersion == 28) {
queries.push('ALTER TABLE folders ADD COLUMN `icon` TEXT NOT NULL DEFAULT ""'); // 1: Folder, 2: Icon
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the comment (I don't think it's relevant to this line).

@laurent22
Copy link
Owner

  • Please fix the alignment and margins of the text inputs. Please remove the big gap to the right to the right of the dialog:

image

  • Please use the same icons has in the Note properties dialog to save the field data (the one with the pen). It's ok if you just copy and paste the code from there to your properties dialog.

image

  • Does the feature work? I've set the icon name to "lock" but it just displayed nothing in the sidebar.

  • In the sidebar the notebook titles should be aligned with tag titles

  • How does the user know what icon name can be used?

@bedwardly-down
Copy link
Contributor Author

I am sorry to do this to you after coming as far as I have, but I would like to take a step back from contributing and being a member of this project outside of being a user and sponsoring like I currently am. I came to the realization last night as I was packing up my laptop at the coffee shop I spent the night at that I'm obsessing too much over this one contribution and this project from an unhealthy angle that introduces a toxic element into the fold. Since this is my first contribution here and I haven't been a part of this community for long, I'd like to keep my dignity and give reigns over to someone who can make a positive difference here.

I thank you for understanding.

@laurent22
Copy link
Owner

No problem, and thanks for giving it a try anyway. To be fair, it wasn't an easy first issue, I think it was a bit mislabelled as there was a lot of concepts to grasp, yet you went quite far through it! It also allowed us to better understand the issue and how it should be implemented (for example I would have done the dialog differently, with just one Save button, but your approach makes more sense actually), so well done.

Let's close the pull request then, and it can always be a starting point if someone else wants to take over.

@laurent22 laurent22 closed this Feb 11, 2020
@laurent22 laurent22 mentioned this pull request Feb 28, 2020
Open
@rabeehrz rabeehrz mentioned this pull request Feb 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurent22 laurent22 laurent22 left review comments

Assignees
No one assigned
Labels
desktop All desktop platforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bedwardly-down @laurent22 @tessus