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

UI Spring Cleaning: Note info panels #2622

Merged
merged 8 commits into from
Apr 8, 2021
Merged

UI Spring Cleaning: Note info panels #2622

merged 8 commits into from
Apr 8, 2021

Conversation

codebykat
Copy link
Member

@codebykat codebykat commented Feb 3, 2021 •
edited by sandymcfadden
Loading

Fix

  • Split note info component into two components, NoteInfo and NoteActions.
  • Remove publish note info from Share dialog
  • Remove redundant icons from the note toolbar

Fixes #2569

Known issues:

  • There's a bug where if you check a checkbox repeatedly. This has to do with updates to the note coming back from GAE. The timing can be off and GAE will send an update basically undoing one of the changes in-app. This isn't something we are going to fix right now as it would involve substantial work in GAE. Thanks, @dmsnell for helping track this down.
  • Dark mode colors aren't 100% and will be updated separately.

Screenshots

Before:

Light Mode Screen Shot 2021-03-22 at 9 56 02 AM
Dark Mode Screen Shot 2021-03-22 at 9 56 15 AM

After:

Light Mode

Note info w/ references Screen Shot 2021-03-22 at 9 50 56 AM
Note info no references Screen Shot 2021-03-22 at 9 51 09 AM
Note Actions Screen Shot 2021-03-22 at 9 52 21 AM

Dark Mode

Note info w/ references Screen Shot 2021-03-22 at 9 50 39 AM
Note info no references Screen Shot 2021-03-22 at 9 51 23 AM
Note Actions Screen Shot 2021-03-22 at 9 51 56 AM

Test

  1. Open a note
  2. Notice new note toolbar
  3. Click on the new info (i) icon
  4. Notice the new info dialog
  5. Click the new actions (...) icon
  6. Ensure all options work as expected
  7. Smoke test in light and dark mode ensuring styles are correct

Release Notes

  • Updates the note toolbar and note info sidebar to be two dialogs, one for note info, and one for note actions.

@codebykat codebykat added this to the 2.7.0 milestone Feb 3, 2021
@codebykat codebykat force-pushed the update/spring-cleaning-note-info branch from abef657 to 54a4044 Compare February 3, 2021 07:38
@codebykat codebykat modified the milestones: 2.7.0, 2.8.0 Feb 5, 2021
@codebykat codebykat force-pushed the update/spring-cleaning-note-info branch 4 times, most recently from 0e0a031 to fe2b8eb Compare February 12, 2021 00:18
@codebykat codebykat force-pushed the update/spring-cleaning-note-info branch from b625b16 to 085b2e8 Compare February 24, 2021 22:38
@codebykat
Copy link
Member Author

SVG checkbox backgrounds: checkboxes.zip

Currently known issues:

  • Checkboxes need these svgs as background
  • Dark mode background colors don't look correct
  • Checkboxes aren't vertically aligned correctly

@sandymcfadden sandymcfadden self-assigned this Mar 9, 2021
dcalhoun
dcalhoun reviewed Mar 9, 2021
View reviewed changes
Copy link
Member

@dcalhoun dcalhoun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I know this still a work in progress, but figured I'd provide some thoughts that may be helpful as we work to complete this.

lib/note-actions/index.tsx Outdated Show resolved Hide resolved
lib/note-actions/index.tsx Outdated Show resolved Hide resolved
lib/note-actions/index.tsx Outdated Show resolved Hide resolved
lib/note-actions/index.tsx Outdated Show resolved Hide resolved
lib/note-info/index.tsx Show resolved Hide resolved
@codebykat codebykat modified the milestones: 2.8.0, 2.9.0 Mar 10, 2021
@dcalhoun
Copy link
Member

I noticed that 02b338c heavily modifies the package-lock.json. It would appear it changes the lock file version, which I believe occurs if you switch from npm@6 to npm@7. Just wanted to call attention to this to make sure this was an intentional change. The GB Mobile team ran into a few issues that delayed us from moving to npm@7 for a little while.

@sandymcfadden
Copy link
Contributor

Thank you so much @dcalhoun for catching that and helping me fix it. You were totally right I was using npm@7 and now back to npm@6.

@sandymcfadden sandymcfadden force-pushed the update/spring-cleaning-note-info branch from 863916f to d664f82 Compare March 22, 2021 12:54
@sandymcfadden sandymcfadden changed the title [WIP] UI Spring Cleaning: Note info panels UI Spring Cleaning: Note info panels Mar 22, 2021
@sandymcfadden sandymcfadden requested review from SylvesterWilmott and a team March 22, 2021 13:43
@sandymcfadden sandymcfadden marked this pull request as ready for review March 22, 2021 13:44
@SylvesterWilmott
Copy link
Contributor

Looks good! Thanks @sandymcfadden

A couple of requests:

  • When publish is pressed in the note options, can we position the spinner on the right hand side of the row and at the same size and colour as the checkbox icons.
  • Pressing the "..." icon in the editor always opens the note options, even if it's already open. Instead, the "..." icon should be a toggle for the menu where pressing it a second time should hide the note options menu.
  • In the note info, can we add the synced time as the top-most item and change the label to "Last synced" to align with Android.
  • In the note options, let's keep the interaction with each row consistent. In the rows with the checkbox we require a click in the checkbox itself. Let's make it so anywhere in the row can be clicked.

@sandymcfadden
Copy link
Contributor

Thank you @SylvesterWilmott I have those items addressed now.

@SylvesterWilmott
Copy link
Contributor

Looks great!

One last thing, can we position the options menu higher up? It looks like it's positioned so it doesn't overlap with the tooltips but it's okay if it does. The menu should be positioned 16px relative to the bottom of the toolbar (an approximation is fine).

@sandymcfadden
Copy link
Contributor

Thanks again @SylvesterWilmott I have that position updated now as well.

@codebykat
Copy link
Member Author

Yayyy this is looking very good!

I have some doubts about the checkbox color in dark mode, it seems very dark:

Screen Shot 2021-03-25 at 7 15 04 PM

Here it is in light mode:

Screen Shot 2021-03-25 at 7 22 34 PM

@SylvesterWilmott
Copy link
Contributor

Nice spot @codebykat

The colour of these icons in dark mode should be gray30, currently they are set at gray50.

@sandymcfadden
Copy link
Contributor

Thank you both @codebykat and @SylvesterWilmott I had the colors reversed for light and dark mode. I have it fixed up now.

@codebykat
Copy link
Member Author

Looks great now, I love it! :shipit:

@sandymcfadden sandymcfadden modified the milestones: 2.9.0, 2.10.0 Mar 30, 2021
codebykat and others added 8 commits April 8, 2021 09:06
@codebykat @sandymcfadden
split into two components, mostly get it working, needs lots of love …
c6414db 
…still

dark theme on note info, dark theme hover on note actions, remove icons from menubar

remove publish tab from sharing dialog

ignore clickoutside on the icon, remove unused icons

still terrible but slowly getting less terrible

switch position of preview and checklist icon

make publish checkbox work

checkbox styling

lint

adjust note info styles, cross icon, reference link titles

menu item and order changes

Add checkbox styles

Switching from using react-clickoutside to focus-trap-react

Changing clickable divs to buttons

Fix linting error in css

Switch from onClickOutside to focus-trap-react on the note info dialog

Fix package-lock.json file

Improve note Action and Info focus management (#2737)

    Remove redundant focus management within note Info. React Modal provides its own focus trap logic, so wrapping with FocusTrap is unnecessary.
    Support ClipboardButton within focus trap for note actions. In order for clipboard.js to function properly within a focus trap, we must set a container that is "reachable" from within the focus trap.

Update styles for when history is disabled

Update note info styles to match designs.

Update border and boxshadow of note actions

Updates to the note action styles

Add loading state for publishing note

Remove commented out no longer needed styles

Fix css linting error

Testing a throttle on the pin note action

Removing the throttle added in the previous commit

Update styles for note info dialog
@sandymcfadden
Update some more note info styles to match designs
637d716
@sandymcfadden
Don't close note info when clicking off of it. Must use close icon
4fdbd97
@sandymcfadden
Fixing publish action spinner position and testing closing the note a…
80feb91 
…ctions
@sandymcfadden
Set a delay on closing the note action menu
3439443
@sandymcfadden
Add pointer cursor styles to have actions be consistent
53502d1
@sandymcfadden
Update position of the note action menu
732e2d3
@sandymcfadden
Fix colors of the note action checkboxes
eba6779
@sandymcfadden sandymcfadden force-pushed the update/spring-cleaning-note-info branch from 44940fb to eba6779 Compare April 8, 2021 12:13
@sandymcfadden sandymcfadden merged commit 54b2428 into develop Apr 8, 2021
@sandymcfadden sandymcfadden deleted the update/spring-cleaning-note-info branch April 8, 2021 12:54
This was referenced Apr 9, 2021
Merged
Merged
@sandymcfadden sandymcfadden mentioned this pull request Apr 20, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandymcfadden sandymcfadden sandymcfadden left review comments

@dcalhoun dcalhoun dcalhoun left review comments

@SylvesterWilmott SylvesterWilmott SylvesterWilmott approved these changes

Assignees

@sandymcfadden sandymcfadden

Labels
None yet
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.

UI spring cleaning: note info
4 participants
@codebykat @dcalhoun @sandymcfadden @SylvesterWilmott