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.

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

Simple search UI #151

Merged

Conversation

LotteHofstede
Copy link
Contributor

This PR connects to #136
The simple search UI was implemented in this PR.
The search UI is split up in two components: search results and the search form

The search results are a list of items found by the search service. The results use the object list component (with pagination) to show all found objects. Special object list elements were created for search results, so hithightlighted values (with markup) are displayed in the search results instead of the default metadata field value.

The search form is the input form on top of the search page. This page can be reused on other pages (for example, I also added it to the home page). The form exists out of a dropdown, where a scope can be selected when available and an input form, where the user can type a search query.

Bootstrap was upgraded to Bootstrap4 beta - there are still be some minor issues with @ng-bootstrap 4 beta - that will hopefully be resolved in the future.

There still seems to be something wrong with the tests, I will try and fix this as soon as possible.

…e-search-with-select

Conflicts:
	src/app/search-page/search-page.component.ts
…arch-with-select

Conflicts:
	package.json
	src/app/core/cache/builders/remote-data-build.service.ts
	src/app/shared/shared.module.ts
@ghost ghost assigned LotteHofstede Sep 21, 2017
@ghost ghost added the needs review label Sep 21, 2017
@artlowel
Copy link
Member

This PR connects to #140

That issue has a comment saying we shouldn't upgrade bootstrap now, but rather wait until the css only version is released. In today's meeting discussed it, and decided to upgrade now and switch to bootstrap-css when it becomes available

Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

While this code looks good overall, I wish it had new specs/tests (for new components added). It seems to only update existing specs/tests to ensure they pass.

Is it time to start talking about requiring specs, @artlowel? We may be able to even find a tool that auto-checks if test coverage decreases per PR (the goal being that each PR needs to ensure no decrease in coverage)?

While I know it takes longer to develop new features w/specs, I worry we're starting to ignore specs (and they'll be difficult to go back and recreate after the fact).

@ghost
Copy link

ghost commented Sep 29, 2017

@tdonohue I suggest coveralls for coverage report and integration with travis ci build.

https://coveralls.io/

@artlowel
Copy link
Member

I've looked at the conflicts between the three PRs, and it seems to be it would be easiest to merge this one first, then #152 and then #149

So I propose we merge this without tests, but leave the issue open, and ask @LotteHofstede to add those tests in a separate PR as soon as possible

I have no experience with coveralls, but looking at their site it seems to do the job, so +1 for that

@ghost
Copy link

ghost commented Sep 29, 2017

Sounds good to me. I will resolve merge conflicts on #152 as soon as this is merged.

@tdonohue
Copy link
Member

@artlowel : That plan sounds fine to me. I'll also create a ticket to configure coveralls. As I do think it's time to start ensuring we are building out specs alongside new code (and adding specs for old code that doesn't yet have it).

@artlowel artlowel merged commit dd2b8b7 into DSpace:master Sep 29, 2017
@ghost ghost removed the needs review label Sep 29, 2017
@LotteHofstede LotteHofstede mentioned this pull request Oct 4, 2017
@tdonohue tdonohue added this to the 7.0preview milestone Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants