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

Undocumented/journey smart search #1927

Merged
merged 27 commits into from
May 27, 2024
Merged

Undocumented/journey smart search #1927

merged 27 commits into from
May 27, 2024

Conversation

kaulfield23
Copy link
Contributor

@kaulfield23 kaulfield23 commented Apr 23, 2024 •
edited

Description

This PR implements journey subject filters in smart search.

Screenshots

image
image
image
image

Changes

  • Adds DisplayJourney and Journey
  • Adds journey filter type in FilterEditor and FilterGallery
  • Adds messages related to journey filters

Notes to reviewer

Related issues

Undocumented

@kaulfield23
create gallery for journey. Render add/remove/limit inputString to jo…
ae81d7d 
…urney component
@kaulfield23
add open/close operator
6e2023d
@kaulfield23
add journeySelect, give minWidth to styledselect
b8b2b55
@kaulfield23
add timeframe at the end of the sentence
dc0fffd
@kaulfield23
change icon on display journey in overview, create DisplayJourney boi…
95d5455 
…ler template
@kaulfield23
render selected options to display
6aa1c5e
@kaulfield23
disable button when there is no selected journey title
a483300
@kaulfield23
change open to opened
4b819f3
@kaulfield23
add condition to message, key tags, condition and min_matching to fil…
1cc88d7 
…ter config
@kaulfield23
add tags to message, rendering tags in DisplayJourney
e66b4f7
@kaulfield23
add 'any tags' to drop down option,change messages, create enum JOURN…
85937c6 
…EY_CONDITION_OP
@kaulfield23
tag description texts before tags
4f83d05
@kaulfield23
add 'of' right after the min matching input number
f7f7c4e
@kaulfield23
remove condition.edit in message and relevant codes
7fc01c6
@kaulfield23
remove minMatch useState and useEffect for it
99fef54
@kaulfield23
hide 'has' when 'regardless of tags' selected
efa5437
@kaulfield23
change tags type to object that contains other props and fix codes
13dfc21
@kaulfield23
use selectedTags instead of making another variable
b7eff31
@kaulfield23
change journeys to journey_subjects
d094c93
@kaulfield23
change min_match to some
0a5473a
@kaulfield23 kaulfield23 marked this pull request as draft April 23, 2024 14:49
@niklasva82
Copy link
Member

This has been confirmed to work with the backend now.

The only issue is that the tags field is reset every time you select a new condition. I don't think this should be the case.

@kaulfield23
Copy link
Contributor Author

Yay!! Thank you @niklasva82 ! I will fix that!

@kaulfield23 kaulfield23 marked this pull request as ready for review May 3, 2024 08:00
ziggabyte
ziggabyte requested changes May 27, 2024
View reviewed changes
Copy link
Contributor

@ziggabyte ziggabyte left a comment

Choose a reason for hiding this comment

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

I played around with it and the only major buggy behaviour I found is on "has at least" , which is also covered in #1948.

I found another tiny visual thing, where he tags could use a bit of space above, since they are colliding with the text underlines on the row above them:
bild

Other than that what I could find was minor code refactor things :)

src/features/smartSearch/components/filters/Journey/index.tsx Outdated Show resolved Hide resolved
src/features/smartSearch/components/filters/Journey/index.tsx Outdated Show resolved Hide resolved
src/features/smartSearch/components/filters/Journey/index.tsx Outdated Show resolved Hide resolved
src/features/smartSearch/components/filters/Journey/DisplayJourney.tsx Outdated Show resolved Hide resolved
src/features/smartSearch/components/filters/Journey/DisplayJourney.tsx Show resolved Hide resolved
@kaulfield23
Copy link
Contributor Author

kaulfield23 commented May 27, 2024 •
edited

About that overlapping style, I just checked personTag and it has the same problem as well. I'm going to make a new issue for it!

@kaulfield23
Copy link
Contributor Author

Hmm should I fix 'has at least' bug in this PR as well? 🤔

@ziggabyte
Copy link
Contributor

Hmm should I fix 'has at least' bug in this PR as well? 🤔

No, let's leave it! If you want you can make a comment on Anton's pr that he remembers to add this logic also to the Journey filter when he's got it down :)

ziggabyte
ziggabyte approved these changes May 27, 2024
View reviewed changes
Copy link
Contributor

@ziggabyte ziggabyte left a comment

Choose a reason for hiding this comment

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

Looks great to me :)

@kaulfield23 kaulfield23 merged commit deaf96f into main May 27, 2024
5 checks passed
@kaulfield23 kaulfield23 deleted the undocumented/journey-smart-search branch May 27, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ziggabyte ziggabyte ziggabyte approved these changes

@richardolsson richardolsson Awaiting requested review from richardolsson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kaulfield23 @niklasva82 @ziggabyte