-
Notifications
You must be signed in to change notification settings - Fork 0
feature mx-1888 adds infobox to the ingest page #368
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, congrats to your first mex pull request!
AUX_PROVIDER_ORDIC, | ||
AUX_PROVIDER_WIKIDATA, | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know you wrote you didn't like enums, but i also think that's the only way of avoiding to write the strings more than once
class AuxProvider(Enum):
"""Allowed auxiliary providers."""
LDAP = "ldap"
ORCID = "orcid"
WIKIDATA = "wikidata"
@@ -137,7 +141,18 @@ def search_input() -> rx.Component: | |||
autofocus=True, | |||
max_length=100, | |||
name="query_string", | |||
placeholder="Search here...", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these new placeholders don't really fit in the width of the search box, could you either revert the change, shorten the placeholder text or widen the input element?

if you decide to keep them, you would need to update the failing test. but maybe it's more stable to match the input field by testid rather than by placeholder anyway.
so instead of page.get_by_placeholder("Search here...")
you could use page.get_by_test_id("aux-search-input")
and then add the testid to the input like this rx.input(..., custom_attrs={"data-testid": "aux-search-input"})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the dynamic placeholder stuff due to its not part of the ticket and i might have done a bit too much.
'Search Wikidata by "Concept URI". Please paste URI e.g. "http://www.wikidata.org/entity/Q918501".' | ||
), | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a test in tests/ingest/test_main.py
please to verify callouts (and placeholders) show the correct text for the selected provider? (at least for one or two of the providers)
PR Context
Added a type for the different aux values. Maybe its a bit overengineered but now u cant say sth like 'set_current_aux_provider("best_aux_ever")'. Would be lovely if we can achieve that the string "ldap", "orcid", "wikidata" are only written once (DRY). Only found a solution with an enum, which i didn't like.
Added
Changes
str
toAuxProvider