Skip to content

Conversation

@DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Jun 2, 2023

☑️ Resolves

🖼️ Screenshots

🏚️ Before 🏡 After
image image

Default View
image

Focused on input or during the textual search
image

🚧 Tasks

  • Visual review
  • Code review

🏁 Checklist

@DorraJaouad DorraJaouad force-pushed the unread_mentioned-_filter branch from e309490 to 66f3010 Compare June 2, 2023 17:33
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

PR looks a bit messy now. Please, figure out the problem with Eslint and put the code in order.
And don't hesitate to ask questions!

@Antreesy
Copy link
Contributor

Antreesy commented Jun 5, 2023

Also tests should be adjusted:
https://github.com/nextcloud/spreed/actions/runs/5176171012/jobs/9324649445?pr=9688#step:7:2218

look through the project to see, how findNcActionButton helper is used in tests, and fix spec.js file
You could run tests locally, see scripts in package.json

@DorraJaouad DorraJaouad force-pushed the unread_mentioned-_filter branch from bed8191 to 498c4ad Compare June 5, 2023 19:39
@DorraJaouad DorraJaouad marked this pull request as ready for review June 6, 2023 07:20
@Antreesy
Copy link
Contributor

Antreesy commented Jun 6, 2023

I reverted some codeline wrapping changes and fixed a couple of things: 2865709
You could cherry-pick this commit and apply, to reduce amount of lines changed.

Also could be done:

  • squash commits into one (or two commits: one for new functionality, one for refactoring and moving NewGroupConversation component
  • we may get rid of EventBus.$on('new-group-conversation-dialog') and call showModalForItem directly from LeftSidebar (see how it's done in other component):
    this.$refs.newMessage.focusInput()

@DorraJaouad DorraJaouad force-pushed the unread_mentioned-_filter branch from 8b803b0 to f6ca400 Compare June 7, 2023 10:35
@DorraJaouad DorraJaouad mentioned this pull request Jun 8, 2023
3 tasks
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Nice! A detail: The sorting of the action menu entries should be the other way around, cause currently the least important (unread messages) is up top.

Also, could you add a screenshot of how it looks like when you have it filtered by e.g. mentions and the action menu is closed? (Needs to be obvious it is in a non-default state and offer a one-click reset, e.g. by showing a chip "Unread mentions" with an x to reset.)

@jancborchardt
Copy link
Member

Ah just saw in the issue it will be showing "is:unread" in the search field so that would answer it. It is quite technical though, but something for a follow-up.

@DorraJaouad
Copy link
Contributor Author

Nice! A detail: The sorting of the action menu entries should be the other way around, cause currently the least important (unread messages) is up top.

Also, could you add a screenshot of how it looks like when you have it filtered by e.g. mentions and the action menu is closed? (Needs to be obvious it is in a non-default state and offer a one-click reset, e.g. by showing a chip "Unread mentions" with an x to reset.)

Thank you. The filter gets cleared by clicking on the x button from the search box.
image

@Antreesy
Copy link
Contributor

Antreesy commented Jun 8, 2023

Ah just saw in the issue it will be showing "is:unread" in the search field so that would answer it.

We asked about it in the comment above, actually. Should the query requests like this be language-independent (like Github uses it), or translated for users?

  • is:unread
  • t('spreed', 'is:unread')

I'd prefer first option, as we are currently using the string match to check if we need to filter results or just search, and translation in current state could break this feature, but method could be adjusted, it's up to user experience.

@DorraJaouad DorraJaouad force-pushed the unread_mentioned-_filter branch 2 times, most recently from 80e58e3 to 4978e90 Compare June 12, 2023 14:54
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Apart from couple of comments, look nice and clean
Great job!

</NcActions>

<!-- New Conversation -->
<NewGroupConversation :show-modal="isNewGroupConversationOpen"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, we could use the shorthand property :show-modal.sync here, like in this commit: 82545ba

But we shoul emit update:show-modal with true/false parameter in that case

@Antreesy Antreesy requested a review from ShGKme June 12, 2023 15:18
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I think it's not very good that we're mixing the creation of a new conversation with the filtering of the conversations. This functionality should be done in a separate filtering button.

| search field | filtering options action button | new conversation action button (in the future could also contain join open conversation action button) |

The search field can expand to the whole width of the line when clicked and the other elements can hide during the search operation

blocking because I think that further design conversation is needed @nextcloud/designers

@DorraJaouad
Copy link
Contributor Author

The design team suggested adding a divider between filters and other options ( there is a 4th option coming up) in this issue which seems good.

At first thought, I had this idea in mind for the filters. We can put the new conversation action button and the list of open conversations action button together in a drop-down list instead of the '+'

@marcoambrosini
Copy link
Member

cc @jancborchardt @nimishavijay @szaimen I think this deserves its own button, also to display active state when filtering is active

Signed-off-by: DorraJaouad <[email protected]>

Fine tuning part 2

Signed-off-by: DorraJaouad <[email protected]>

LeftSidebar test correction

Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad DorraJaouad force-pushed the unread_mentioned-_filter branch from 333ccf9 to 5d0530f Compare June 26, 2023 15:28
Signed-off-by: DorraJaouad <[email protected]>
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

I don't think that the search box should be disabled when a filter is active, why are we doing that? The search box should stay clickable so that the search operation can be initiated immediately without worrying about filters.

@DorraJaouad
Copy link
Contributor Author

I don't think that the search box should be disabled when a filter is active, why are we doing that? The search box should stay clickable so that the search operation can be initiated immediately without worrying about filters.

For example, the user can type a text in the search box and activates a filter right after. As the text remains there, It will be mistakenly thought that the search text is used to filter out the search output. Vice versa, if the search box remains clickable, the user can try to write a textual filter while the filter is active.

  1. the search filter can not be applied together with a textual filter

@nickvergessen
Copy link
Member

For example, the user can type a text in the search box and activates a filter right after. As the text remains there, It will be mistakenly thought that the search text is used to filter out the search output. Vice versa, if the search box remains clickable, the user can try to write a textual filter while the filter is active.

I commented this in the beginning as well. It would be cool to to be able to filter and additionally still search. Should be doable?

@DorraJaouad
Copy link
Contributor Author

Yes, we can still amend it but the whole feature was kinda built on the fact that both textual and new filters cannot be combined together. The search box expansion was added purposely to hide the filters during the search operation. Also, there was a reason behind this separation.

..For the purpose of talk search, I don't think we need filtering of the search results. That would be also a bit misleading because the search results are not only joined conversations, but also not joined ones, for which there 's no read/unread state.
#9688 (comment)

I would change it if you all think it is better to combine them. It was like that in the very beginning.

@marcoambrosini
Copy link
Member

the user can type a text in the search box and activates a filter right after

@DorraJaouad this shouldn't be possible because as long as there's a search operation undergoing we are not displaying the filter button.

@nickvergessen As I raised previously I think that mixing filtering and searching functionality would complicate things a lot and I see very little benefit to it.

This is what I would suggest as a first iteration of this:
When a filter is active and the user initiates a search, we should just disregard the filter and return the search results normally. Once the user selects a search results we go back to whatever filtering option the user had previously selected.

Also, we shouldn't forget placing an empty content view if the filtering doesn't return any conversation :)

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

#9688 (comment) I think that mixing filtering and searching functionality would complicate things a lot and I see very little benefit to it.

Alright, was not aware of this since there were too many comments when I returned and got asked. Also makes a bit of sense, so lets see how that feels naturally.

When a filter is active and the user initiates a search, we should just disregard the filter and return the search results normally. Once the user selects a search results we go back to whatever filtering option the user had previously selected.

Should be easily doable by making the switch block an if-statement with the search as first branch.


My final pain point would be that the + button should stay visible when searching. If you can not find the conversation you are looking for, you will have to create it, but for that you (currently) first have to clear the search term, which is rather unexpected (in the past we even had the feature that the current search term was automatically prefilled as room name, but that seems to be broken even on stable27)


After this we should get the PR in and then consider doing follow ups, to not drag this PR longer than needed.

@nickvergessen
Copy link
Member

Also, we shouldn't forget placing an empty content view if the filtering doesn't return any conversation :)

This is already there

@marcoambrosini
Copy link
Member

@nickvergessen how about reintroducing that feature as the last search result + empty content of a search instead of leaving the + button visible?

@nickvergessen
Copy link
Member

nickvergessen commented Jun 28, 2023

Then let's move that to a follow up too.

So @DorraJaouad todo:

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I think we need some padding between the button and the input, but otherwise good to go from my side:
grafik

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Works and looks good, visually and by the code
Nothing blocking from my side, what we couldn't done later (but I left some commits to consider as a follow-up)

Signed-off-by: DorraJaouad <[email protected]>
@DorraJaouad DorraJaouad force-pushed the unread_mentioned-_filter branch from 2bcf902 to 4354691 Compare June 29, 2023 11:44
@DorraJaouad DorraJaouad enabled auto-merge June 29, 2023 14:51
@DorraJaouad DorraJaouad dismissed marcoambrosini’s stale review June 29, 2023 17:03

Sufficient approvals reached

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks nice! :)

@DorraJaouad DorraJaouad merged commit 3b58ab9 into master Jun 29, 2023
@DorraJaouad DorraJaouad deleted the unread_mentioned-_filter branch June 29, 2023 19:30
@DorraJaouad
Copy link
Contributor Author

/backport to stable27

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Frontend - Filter conversation list for unread / mentioned conversations only

7 participants