Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

PLAT-60 search display view modes #142

Merged
merged 13 commits into from
Apr 13, 2016

Conversation

AndyEPhipps
Copy link
Contributor

@bimsonz
Copy link
Contributor

bimsonz commented Apr 7, 2016

@AndyEPhipps so them problem here is that because you've changed the view modes for the search, the test for search no longer pass...

We need to update the test as part of this branch, @wagg-matt are you able to assist andy with this? need to look at what best to test so this doesnt have to change everytime the viewmode is altered slightly? imo this should be a functional test, maybe test to see if markup is there or something ? /FYI @pvhee

@gusliedke
Copy link
Contributor

Yeah, maybe do a bit more generic test for elements / functionalities that aren't finished yet.

@wagg-matt
Copy link
Contributor

@bimsonz of course. @AndyEPhipps give me a shout when you want to go through it and i'll head over

@gusliedke
Copy link
Contributor

@AndyEPhipps Let's try to achieve this markup.

We can make one template for all search results and one file for its style.

<div class="search-result">
    <h2 class="search-result__title"></h2>
    <div class="search-result__image">
        <img src="#" alt="image title">
    </div>
    <a class="search-result__link" href="#">Read more</a>
</div>

Sass file can have same name as element. ie. _search-result.scss

@bimsonz bimsonz changed the title Feature/plat 60 search display view modes PLAT-60 search display view modes Apr 12, 2016
@pvhee
Copy link
Contributor

pvhee commented Apr 12, 2016

@AndyEPhipps @wagg-matt @bimsonz I fixed the behat tests (let's wait for travis to confirm), see commit 051c8d5 - don't think we have to be too worried about front-end changes for the future, the point is that when you look for "malaria" you should return two pages, and when looking for "lindsey" you also have two results (both of them not articles but landing pages). Content is part of the build, so that's what we should be asserting, and at the same time this checks that the entire search stack is functioning correctly.

…o feature/PLAT-60_search_display_view_modes

Conflicts:
	profiles/cr/modules/custom/cr_search/config/install/search_api.index.cr_content.yml
@pvhee
Copy link
Contributor

pvhee commented Apr 13, 2016

@AndyEPhipps is this ready to go in now?

@peter-brady
Copy link
Contributor

@AndyEPhipps @bimsonz @pvhee

Thinking dependency hell here, to be honest. Here's why.

We have these view modes defines in the search module. They view modes are for the Partners node type, and a few others. Thing is, does that mean this module depends on the Partners content type being available? Because it's likely that some sites won't have a Partner content type enabled Eg. comicrelief.com for example.

Thinking we should do a little more testing in the area of dependencies before we start getting too many modules with all these dependencies into the codebase.

What do you think?

@@ -0,0 +1,10 @@
{#
Copy link
Contributor

Choose a reason for hiding this comment

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

@AndyEPhipps I don't think we need to commit add this file.

@gusliedke
Copy link
Contributor

@AndyEPhipps I think there are some files that need to be deleted. I left a comment on them.

@AndyEPhipps AndyEPhipps merged commit 8efc392 into develop Apr 13, 2016
@AndyEPhipps AndyEPhipps deleted the feature/PLAT-60_search_display_view_modes branch April 13, 2016 14:43
@pvhee
Copy link
Contributor

pvhee commented Sep 23, 2016

@gusliedke @AndyEPhipps just a FYI that we'll need to redo some of the work to make sure the search view modes are updated to the new look & feel - currently this looks all a bit broken (as expected)

Related to our main search PR: #531

@gusliedke will probably pop in a new ticket for the front-end work required

@gusliedke
Copy link
Contributor

We have this one for search front end work https://jira.comicrelief.com/browse/PLAT-485

@pvhee pvhee mentioned this pull request Jan 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants