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

Fix gridded yxt-Card styling for 2 columns #784

Closed
wants to merge 53 commits into from

Conversation

creotutar
Copy link
Contributor

Fixes #783

We should be applying width to the Card for 2 columns just as we are doing for 3 and 4 columns.

nbramblett and others added 30 commits April 24, 2020 19:32
The navigation param in the base config is called verticalPages now, and
is an array rather than an object.

TEST=none
* Always use CustomEvent polyfill

This commit changes DOM.trigger to always use our custom event
polyfill, instead of looking in the window first to see if
CustomEvent exists as a function.

Consulting was running into issues with ie11 saying that
it doesn't know how to use the Event constructor. This likely
could have also been fixed by changing `return new Event` to
`return new CustomEvent`, however it felt more robust to me
to always rely on our own polyfill, instead of something
somebody using the sdk adds.

TEST=manual
Went on ie11, checked that adding a CustomEvent function onto the
window manually through the dev tools causes the error consulting
was seeing. This was checked by typing into the searchbar,
since the searchcomponent uses DOM.trigger.
Then, removed code and checked that there were no errors,
and autocomplete was displaying values that I could click on
to run a search. Also checked on chrome.
This PR adds the infrastructure needed to write TestCafe acceptance tests for
the SDK. It also creates the first such test. This acceptance test is
relatively simple and verifies a basic flow on a universal search page.
Essentially, it types a manual query, clears it, picks an autocomplete option
and verifies that the expected VerticalResults sections are there. This test
is a jumping off point and is by no means exhaustive. We should look to add
more to this test, test other flows, and make sure we have Block models for
every component. Right now, tests are only run in Safari and Chrome. Firefox
support and IE11 support we come in later PRs.

J=SPR-1963
TEST=auto

Made sure the new acceptance test could pass. Watched as the test ran in both
Safari and Chrome to make sure the test matched what I thought it should be
doing.
The correct Selector was not being used to select the various autocomplete
options in the DOM. This PR corrects that.

TEST=auto

Reran acceptance test and made sure it still worked.
This PR ensures that the SearchBar component has a border when viewed on
Mobile. The border is the same as the one that already exists on the Desktop
view. Without a border, the SearchBar's input was impossible to find against
a white background on Mobile.

J=SPR-2001
TEST=manual

Verified that the border appears the same accross Desktop and Mobile in both
IE11 and Chrome.
Update the legacy card to properly reference big date, fixes this issue: #679.

TEST=manual

Locally, create event vertical page and display big date.
Checked for other instances of data-component="IconComponent"
inside #ifs for similar issues, did not find any.

TEST=manual
Tested that can specify a custom iconUrl (using accordion card in vertical results)
with just an iconUrl and no icon option specified
Admins were adding this css by default when migrating to v0.13

TEST=manual
see that vertical on univeral results with useAccordion = true
has this border-top styling
This was causing a breaking change and being overridden manually

TEST=manual
open vertical search page, check that on hover and using the
dev tools hover button the cta does not change color
_config._parentOpts.target is a valid fallback option for standard and
legacy cards now, and the places where the target attribute defaulted
to _blank have been reverted to default to _self.

J=SPR-2025
TEST=manual

Manually verified that setting target in the VerticalResults config
would be reflected in the card links. Also verified that not setting a
target would default to _self instead of _blank.
…700)

For this issue #677 (https://github.com/yext/answers/issues/677), add documentation for the existing UniversalResults config option useAccordion.
When displayAllResults is true, before a page would display
applied filters in the results header, like it does for normal
results.

It's still the case that only nlp filters are displayed.

J=SPR-2003
TEST=manual
Checked that page was displaying applied filters for no results
Made change
checked that filters no longer display for no results
checked that filters still display for regular results
Adds a noResults configuration which alters the behavior of the map component when a vertical search returns no results.

J=SPR-2002
TEST=manual

Verified that inputting a no-results query while displayAllResults was set to false would cause the
map to be empty (and hidden unless otherwise set to display an empty map).
Removed extra bordering on filter component so it looks less bad

TEST=manual

Eye test
Across all browser but internet explorer, searchbar placeholder
text was gray, but on ie11 was black. This was changed to gray.

TEST=manual
check ie11 in browserstack, and that on chrome things look the same
* Make CustomEvent Polyfill match previous non-polyfilled behavior

instead of first trying to use the event constructor, if it exists
on the window, before resorting to the polyfill.

However, this polyfill did not match the behavior of the event
constructor it was replacing, and had cancelable and bubbles set
to false by default instead of true.

Also untrack .DS_Store and add it to the gitignore

TEST=manual
Tested on ie11, firefox, safari, chrome.
Test that typing in search bar and submitting a search still works.
Check that submitting a near me query, then resubmitting another query
does not prompt for the geolocation a second time.
This new option can be set in the top-level Answers.init configuration. It can
be used by both VerticalResults and the MapComponent. Both of these components
have logic that will favor an individual noResults config to the one in
globalStorage. Additionally, both components will apply the same sane defaults
to any noResults attributes not specified. A change was also made to ensure
that the VerticalResults component passes along its noResults config to any
child MapComponent, to ensure consistency.

Note that Rose requested the displayAllResults default be false for the
MapComponent

TEST=manual

Ensured correct behavior in all the following situations:

VerticalResults with included MapComponent

- Specified noResults config for VerticalResults.
- Specified noResults config at global level.
- Specified noResults config at both levels.
- Specified noResults config at neither level.

VerticalResults and separate MapComponent

- Specified noResults config for both components.
- Specified noResults config at global level.
- Specified noResults config at global and component levels.
- Specified noResults config at neither levels.
* update universal results borders

margin on top and bottom of yxt-Results-filters
was not receiving border-left and border-right
(because margin does not receive border in css)
causing gaps to appear

Also fix double border

TEST=manual
check that vertical results on vertical search,
vertical results on universal searchh, accordion results
do not have gaps in the border, and otherwise appearance is unchanged
This change caused an undesired breaking styling change to the
dignity funerals site. The margin-top: 0 and margin-bottom: 0
can be kept, since that was done to override user-agent styling.

TEST=manual
check that padding has same value as before
UniversalResults is dumb and this accommodates the dumbness.

TEST=manual

UniversalResults shows a map now. Same difference for Vertical Results.
If a user enters a cardType that is built in, no changes. If
they enter a cardType that is not built in, the library interprets
it as a component name.

TEST=manual

Update the config to use built in vs custom cardTypes. Observe
changes in browser.
If vertical key is falsy because it is null or empty we should
fall back to the url. Fixes issue where universal's vertical
key was null and it was not showing in nav

TEST=manual,auto

Run local experience and see that universal tab shows up and
component renders without errors.
#535)

If verticalPages config is present in the component config, prefer it over the one in the top-level config.

TEST=manual

Run answers experience locally putting config in top level config, component config, then both.
Pagination component now prefers verticalKey from its component
config. Falls back to the one from answers top-level config.

TEST=manual

Run answers experience locally putting config in top level config, component config, and both.
#578)

If an implementer passed in a verticalKey that was empty or undefined, its
entry would be ignored in the navigation component. Changing the null check
so any falsy verticalKey is treated the same.

TEST=manual

Test on local answers instance that universal verticalPages config is
used when omitting vertical key.
…ds compatibility (#712)

TEST=manual

Replace analytics config option with eventType and notice that analytics
event type is updated.
Legacy card styling was overflowing horizontally. Cards generally
were adding extra white space vertically. This fixes both so cards
in IE11 look the same as other browsers.

TEST=manual

Tested in Chrome, IE11 and Firefox.
TEST=manual

Visually test using dev site and client site on Universal Results page.
alexisgrow and others added 22 commits April 24, 2020 19:32
)

Change so placeholders are not added when the maxNumberOfColumns is 1.
Update placeholder to be an empty element rather than a repeated result card.
The placeholder has the same horizontal margins and border as a card so it has
the same width; add number of placeholders needed - 1 to account for the
requirement that column count changes on screen re-size.

Addresses this issue: #664

TEST=manual

On a Universal page, visually inspect on dev site locally and
client site locally. Test on Vertical page with 4, 3, 2, and 1 columns,
adjusting screen size and noting that the cards stay the same size.
Added for backwards compatibility. The legacy card is the only one
that uses this variable.

TEST=manual

Test locally on Universal and Vertical pages that the config is
passed through and shows in the DOM.
Since we added a border to yxt-Results-titleBar oobx,
and since the titleBar sits on top of yxt-Accordion-list,
yxt-Accordion-list probably needs a border-left and border-right
as well so the results line up with the titlebar.

TEST=manual
test that dignity funerals site has lined up AccordionResults
and yxt-Results-titleBar after updating
Previously the dependency was at version 3.6.4. Dependabot suggested
that we upgrade. This upgrade should not cause any issues as the
new version has the same major and minor revision as the current
version.

J=SPR-2069
TEST=manual

Did a manual smoke test in IE11 since this dependency is used for
the polyfills. Made sure I could navigate between pages, click
autocomplete options, clear a search, submit QA, see a Map, etc.
Ensured there were no console errors.
Dependabot suggested we update the versions of the following @babel plugins:

- @babel/plugin-proposal-object-rest-spread
- @babel/plugin-transform-arrow-functions
- @babel/plugin-transform-runtime

Even though these plugins are devDependencies, they should be kept up-to-date.
We use these plugins to transform our source code when creating the various
asset bundles. It's possible, then, that security issues in these plugins could
make their way into the builds of the SDK. These were only minor version
upgrades, so the risk should be low.

TEST=manual

These @babel plugins are mainly used to help polyfill the SDK's source code and
make it IE11 friendly. To test, then, I ran a few smoke tests on a demo site in
IE11. I made sure search, navigation, spell check, autocomplete, and qa
submission functionality worked and no errors were present in the console. I
also made sure to exercise some areas of the code where we use arrow functions
or the spread operator, to ensure the polyfilling of those continued to work.
Allow an implementer to specify an iconURL in the verticalPages config. This allows them to use an icon that is not in the built-in set that ships with the template bundle. If both iconUrl and icon are passed in, they will both be passed through to the IconComponent which has logic to determine which takes priority.

TEST=manual

In dev site for a Vertical page, use in an iconUrl in the verticalPages config. View in browser to confirm custom icon displays. Also test to verify that built-in icons still work.
* Let url be null on Legacy card.

Let url be null-ed by the user, so that it can be forced to default to
a linkless title.

* Add parens and update standard card
…ages (#737)

On chrome, this lines up ctas perfectly whether there are images or not
On ie11 they still don't line up properly, but I hope this can be a more short-term
fix?

TEST=manual
test with maxNumberOfColumns = 4 and maxNumberOfColumns = 1,
using element.getBoundingClientRect(); for cards with images and without images
* Fix tall mobile cards

* Alexis did it!
…702)

* Update circle ci deploy to also update major version

When deploying a tagged commit, also create and override the major version
for that commit. In other words previously we only stored v1.0 and v1.0.0, now also store v1

TEST=manual
set $CIRCLE_TAG in local bash and tested that was able to export $MAJOR_VERSION
TEST=manual

Proofread the README and check veracity.
This PR allows the SearchBar to be configured to not use a form as the query
submission context. By default, a form is used, unless a client sets the new
'useForm' config option to false. This was done to support clients that wanted
to use a SearchBar on an ASP.NET page.

J=SPR-2175
TEST=manual

Tested the following scenarios for SearchBars set to useForm true and false:

- Entering text and hitting enter.
- Entering text and hitting the submit button.
- Entering text and clearing.
- Clicking autocomplete option.
- Using keys to navigate to and select an autocomplete option.
- Clear button appears in the correct situations.
- Search animation works as expected.
- Near me query works as expected.

In the above scenarios, made sure Analytics worked correctly, if applicable.
#753)

Add modifier option to the VerticalResults portion of the documentation.

TEST=none
Remove hardcoded "DEVICE" accuracy which forces the device accuracy help text.
Add the BSD3 license
The 'limit' here does not accept strings, only numbers. Updating example in the documentation to have the correct type.

TEST=none
…758)

The limit param is not used and is not supported by the back end of Answers for universal searches. Removing the unused param from the README and the component because it is misleading.
Fixes #783

We should be applying width to the Card for 2 columns just as we are doing for 3 and 4 columns.
@creotutar creotutar changed the base branch from master to v0.14.0 May 14, 2020 01:21
@creotutar
Copy link
Contributor Author

Closing to start off of v0.14.0

@creotutar creotutar closed this May 14, 2020
@creotutar creotutar deleted the creotutar-patch-1 branch July 21, 2020 18:49
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.

maxNumberOfColumns does not support 2 columns width
6 participants