Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Edit search label in embed config #1604

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Conversation

emilyhu0106
Copy link
Contributor

@emilyhu0106 emilyhu0106 commented Jan 27, 2022

Overview

Update embedded map UI to add a "Text search label" and search sidebar UI to show custom label which is set on the EmbedConfig

Connects #1534

Demo

Embed Config page

2022-01-27 16 47 41

Embedded Map

image

Testing Instructions

  • Run ./scripts/server and log in as any users with embed permissions (e.g. c2@example.com)
  • Go to the Setting - Embed page to set the search label config
    • The info text is descriptive enough for users
    • When no fields are marked searchable, the custom search box is reset to the default text and disabled.
    • No lag during typing
    • If the text search label is blank, it is replaced with the default text
  • Go to preview and embed mode map
    • The search label changes accordingly

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

@emilyhu0106
Copy link
Contributor Author

In the issue #1534 description we have

Update API to allow passing text_search_label when updating EmbedConfig
Normalize empty string to null

When I am implementing this change, I realize that whenever text_search_label is empty string or null, it will be updated into the default text. Should this normalization be achieved at API level?

@@ -108,7 +108,7 @@ function EmbeddedMapConfigWrapper({ user }) {
} else {
createConfig();
}
}, 500);
}, 1000);
Copy link
Contributor Author

@emilyhu0106 emilyhu0106 Jan 27, 2022

Choose a reason for hiding this comment

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

Update the timeout interval to 1000 ms since this is more practical to finish typing in the text input; 500 ms pause will cause a lag when typing a bit slower

>
Search a Facility Name, OAR ID, or PPE Product Type
</FeatureFlag>
</InputLabel>
<TextField
id={FACILITIES}
placeholder="Facility Name or OAR ID"
placeholder={
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The placeholder text is the same as the text search label when it is embed mode and not default text.

@emilyhu0106 emilyhu0106 force-pushed the eh/update-embed-config-label branch 2 times, most recently from 90f667b to af993dc Compare January 27, 2022 22:19
errors={errors}
/>
<EmbeddedMapSearchLabelConfig
anyFieldSearchable={fields.some(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the search label input component, it only cares about if any field is searchable

@TaiWilkin
Copy link
Contributor

Taking a look now

@TaiWilkin TaiWilkin self-assigned this Jan 28, 2022
@TaiWilkin
Copy link
Contributor

When I am implementing this change, I realize that whenever text_search_label is empty string or null, it will be updated into the default text. Should this normalization be achieved at API level?

I think it's fine that we're managing the change to default text on the UI only, as it allows us to have a single source of truth for the default text.

Copy link
Contributor

@TaiWilkin TaiWilkin left a comment

Choose a reason for hiding this comment

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

As a whole this is working very well. The search text label change in embed config is being applied smoothly and consistently.
I found a couple of anomalies as I was testing the search input:

  • The initial content of the text input is blank, but is updated when I click into it. You might be able to fix that by replacing empty text with the DEFAULT_SEARCH_TEXT here. The downside of that approach is that if they leave the field blank long enough that it auto-saves, the text will get replaced with the default value. You could work around that by preventing autosaving the embed config while the textbox is blank (and only saving a blank field / replacing the text on blur). Let me know if you'd like to pair on that approach.
  • When I enter multiple spaces, then click away from the textbox, it stays blank; however, if I click back into the box and then click out again without changing anything, it is replaced with the default text. I suggest using .trim() on the text when you check to see if the field is blank.

I'm not sure about the descriptive content, so I've tagged @lederer for review to give this a look over as well.

@TaiWilkin TaiWilkin assigned emilyhu0106 and unassigned TaiWilkin Jan 28, 2022
@lederer
Copy link
Contributor

lederer commented Jan 28, 2022

This is neat. Thanks for putting this together. I left some comments.

Currently if no "searchable" checboxes are checked, the input is disabled. This is good! Can you also make the entire subsection grayed out? Maybe just set the opacity of it to, say, 60%.

Copy link
Contributor

@lederer lederer left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes. Few more tweaks requested in some code comments. Almost there!

@emilyhu0106
Copy link
Contributor Author

Thanks for making those changes. Few more tweaks requested in some code comments. Almost there!

I made the changes accordingly. Please take another look, thank you!

Copy link
Contributor

@TaiWilkin TaiWilkin left a comment

Choose a reason for hiding this comment

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

Great job working through those changes! The fix for the default text onChange vs onBlur issue was surprisingly complicated to resolve, but you did a good job working through it.

@TaiWilkin TaiWilkin assigned emilyhu0106 and unassigned TaiWilkin Feb 1, 2022
@emilyhu0106
Copy link
Contributor Author

Thank you both for reviewing!

Update embedded map UI to add a "Text search label" and update search
sidebar UI to show custom label which is set on the EmbedConfig
@emilyhu0106 emilyhu0106 merged commit 659e2be into develop Feb 1, 2022
@emilyhu0106 emilyhu0106 deleted the eh/update-embed-config-label branch February 1, 2022 19:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants