8000 feat: action bar on selected blocks by tiensonqin · Pull Request #11763 · logseq/logseq · GitHub
[go: up one dir, main page]
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

feat: action bar on selected blocks #11763

Open
wants to merge 40 commits into
base: feat/db
Choose a base branch
from
Open

feat: action bar on selected blocks #11763

wants to merge 40 commits into from
+836 −593

Conversation

tiensonqin
Copy link
Contributor
@tiensonqin tiensonqin commented Feb 28, 2025 •
edited
Loading

This PR introduces a new action bar for selected blocks, it also works on table view.

image image

@github-actions github-actions bot added the :type/feature New feature label Feb 28, 2025
@tiensonqin tiensonqin changed the base branch from master to feat/db February 28, 2025 11:26
@tiensonqin tiensonqin marked this pull request as ready for review March 8, 2025 06:05
RCmerci
RCmerci reviewed Mar 10, 2025
View reviewed changes
Copy link
Contributor
@RCmerci RCmerci left a comment

Choose a reason for hiding this comment

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

batch set property not working properly.

  1. select 2 blocks
  2. click 'set property', select a 'icon'
  3. only the first block has the icon

RCmerci
RCmerci reviewed Mar 10, 2025
View reviewed changes
Copy link
Contributor
@RCmerci RCmerci left a comment

Choose a reason for hiding this comment

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

After selecting multiple blocks using 'shift+', the action bar does not appear

@logseq-cldwalker
Copy link
Collaborator

@tiensonqin A couple of bugs I'm seeing:

  • Like Zhiyuan I'm not seeing an action bar when selecting blocks with mouse or keyboard
  • Changing the view type from a table no longer works e.g. table from a property page
  • Setting properties from a table doesn't allow selecting a property value. It seems to only create an empty property value. This feels buggy especially since the table doesn't update to show the changes or there's not at least a notification saying anything changed
  • Cutting from a property table isn't undoable. Should we rename cutting to delete? I was surprised this action deleted properties from the selected rows
  • When doing a batch action from table, I can create a new property but I can't create a new tag. Is that intentional?

Let me know when I should take another look. Also, is there a way to unset a property for selected rows or does that come later?

@tiensonqin
Copy link
Contributor Author

@cldwalker

Like Zhiyuan I'm not seeing an action bar when selecting blocks with mouse or keyboard

It's intentional not to show an action bar with keyboard shortcuts (mod+up/down) because it's distracting and showing an action bar will prevent any further actions in our current implementation (using radix UI).
I think you need to run yarn in packages/ui to see the action bar when selecting blocks with mouse, please record a video if it doesn't work.

Changing the view type from a table no longer works e.g. table from a property page
Setting properties from a table doesn't allow selecting a property value. It seems to only create an empty property value. This feels buggy especially since the table doesn't update to show the changes or there's not at least a notification saying anything changed

I can't reproduce those two bugs, can you provide some reproduce steps? Thanks.

Cutting from a property table isn't undoable. Should we rename cutting to delete? I was surprised this action deleted properties from the selected rows

Cutting from any table will delete the blocks, we can tweak the behavior to remove the current property from the selected blocks if you think that's expected.

When doing a batch action from table, I can create a new property but I can't create a new tag. Is that intentional?

No, it's a bug, I'll push a fix soon.

@tiensonqin
Copy link
Contributor Author

@logseq-cldwalker Thanks for the videos and reproducing steps!

That make sense not to show automatically. It would be helpful for us keyboard users if the actions bar could somehow be activated after selecting blocks e.g. from the right click menu

I updated the behavior to show action bar after selecting blocks with keyboard (shift+up/down), it'll be hidden if there's any further action such as selecting more blocks or using arrows for navigation. I didn't enable action bar after pressing ESC to select a block because I still find it distracting.
I also want to set a shortcut to focus on the action bar when there're selected blocks, any ideas? cc @RCmerci @xyhp915

When I try to use it on two blocks, none of the property types batch add except for :checkbox -

Fixed.

Changing the view type from a table no longer works e.g. table from a property page

I still can't reproduce this one.

Setting properties from a table doesn't allow selecting a property value. It seems to only create an empty property value.

Fixed.

The ux for batch adding from the table feels buggier compared to selecting from blocks. After a batch action in the table, the rows are still selected which makes it feel like the action never took place. Also, none of the added properties display in a table, even if I navigate away and back

Agreed. The added properties are not displayed because only predefined properties are displayed for tags and properties (al properties are shown for query results), showing all properties requires loading all the rows, which may affect the view performance a lot. I plan to revisit this after #11774.

I found the delete button to be better ux b/c I understood I was deleting entities. With cut, I expect undo to work like it does in a normal block view i.e. undo and the cutted blocks reappear in the table. Currently we have to navigate away and back to see the undone table updated

I updated cut to a trash button. There're some general issues with table reactivity, including undo/delete/insert, again, I'd like to address those issues after #11774 because view data loading will be changed a lot in #11774.

@tiensonqin
Copy link
Contributor Author

Also, is there a way to unset a property for selected rows or does that come later?

You can unset :default, :url, :number, :node
image

And :data, :datetime
image

I'm still thinking of a universal approach to handle all types including :checkbox. Let me know if anyone has some ideas.

@tiensonqin tiensonqin requested a review from RCmerci March 11, 2025 02:08
@tiensonqin
Copy link
Contributor Author

@RCmerci The above two bugs are fixed.

@tiensonqin
Copy link
Contributor Author

Changing the view type from a table no longer works e.g. table from a property page

This one should be fixed by 170edfb.

@logseq-cldwalker
Copy link
Collaborator
logseq-cldwalker commented Mar 11, 2025 •
edited
Loading

I updated the behavior to show action bar after selecting blocks with keyboard (shift+up/down), it'll be hidden if there's any further action such as selecting more blocks or using arrows for navigation. I didn't enable action bar after pressing ESC to select a block because I still find it distracting.

Thanks!

When I try to use it on two blocks, none of the property types batch add except for :checkbox -
Fixed.

Thanks for fixing most of these. I'm still seeing :default :many and :url :many as not able to batch set - https://www.loom.com/share/db31921682df4049a0cce97aa1cfadd6. Fwiw I just use the properties example graph to test all these property combinations

Agreed. The added properties are not displayed because only predefined properties are displayed for tags and properties (al properties are shown for query results), showing all properties requires loading all the rows, which may affect the view performance a lot. I plan to revisit this after #11774.

Good to know we're still thinking about this. If it's not happening soon, it would be helpful to just display a notification e.g. Properties added! as the current ux does still feel unresponsive. We don't have this problem with tags because they are always visible

I updated cut to a trash button. There're some general issues with table reactivity, including undo/delete/insert, again, I'd like to address those issues after #11774 because view data loading will be changed a lot in #11774.

Good to know. Thanks for the change for now

This one should be fixed by 170edfb.

I haven't seen this so I think it's fixed

I'm not sure if this is a bug: when I copy on a batch of nodes that include pages, only the blocks get copied - https://www.loom.com/share/941d8989061646908b5dea6a054418ed . If copy can't handle pages maybe it would be helpful to add export edn at some point since it can handle any block type. Happy to do this after the PR if desired

Just this question and the bugs on :url :many and :default :many left

@logseq-cldwalker
Copy link
Collaborator

One other bug I noticed, the property values batch added with a :default or :url property are the same entity, causing the property values to be in sync when edited:

Screen Shot 2025-03-11 at 12 37 21 PM

@tiensonqin
Copy link
Contributor Author

@logseq-cldwalker I've fixed all the issues mentioned above.

when I copy on a batch of nodes that include pages, only the blocks get copied -

Page titles will be copied.

@logseq-cldwalker
Copy link
Collaborator
logseq-cldwalker commented Mar 12, 2025 •
edited
Loading

Confirmed copy bug and most property types fixed. Thanks for adding the notification when updating from the table. Two bugs I'm seeing:

  • With multiple blocks selected, batch add :checkbox properties. Only the first block has the property added
  • From a table of a :url (or :default) :many property, batch add a new value. Expect to see only one instance of it in the dropdown but N number of them appear in the dropdown. Then try to remove the new value. Only the first block has the property value removed. https://www.loom.com/share/66cfd0e23104418eae80fa96139325c0 demos this.

The 2nd bug is tricky because the :many dropdown almost seems in conflict with the data model i.e. we want each property value to be unique but in a batch context we want to pretend like they are the same value. The ui could just display one unique text value and then on delete, delete all the properties with that text value. That would mostly work but could be surprising if two property values have the same content "foo" but different properties, tags or block children. Maybe we leave those edge cases to be handled later

@logseq-cldwalker
Copy link
Collaborator

I'm still thinking of a universal approach to handle all types including :checkbox. Let me know if anyone has some ideas.

I'd missed the whole comment on unsetting. I'll be sure to mention removing properties is found in property values in the docs. One idea for making a universal Remove property approach is provide it as its own action button. When clicked it displays a dropdown of all used properties for the selected nodes. Selecting a property removes that property from all selected nodes

@tiensonqin
Copy link
Contributor Author

With multiple blocks selected, batch add :checkbox properties. Only the first block has the property added

Can you record a video? I'm not able to reproduce this one.

The 2nd bug is tricky because the :many dropdown almost seems in conflict with the data model

Indeed, there are also some cases where it makes sense to set the same block as the property value for multiple blocks. Let's resolve those edge cases when we have more feedback from users.

I'll be sure to mention removing properties is found in property values in the docs.

I added Unset property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xyhp915 xyhp915 Awaiting requested review from xyhp915

@logseq-cldwalker logseq-cldwalker Awaiting requested review from logseq-cldwalker

@RCmerci RCmerci Awaiting requested review from RCmerci

Assignees
No one assigned
Labels
:type/feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tiensonqin @logseq-cldwalker @RCmerci @xyhp915
0