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 distance alignment #1006

Merged
merged 4 commits into from
Nov 16, 2021
Merged

Fix distance alignment #1006

merged 4 commits into from
Nov 16, 2021

Conversation

nmanu1
Copy link
Contributor

@nmanu1 nmanu1 commented Nov 15, 2021

Fix distance alignment for financial-professional-location and professional-location cards so that distance is on the far right of the top row of the card instead of right next to the title. Also, fix distance alignment with the close card button on vertical full page map for both cards and location-standard cards so that distance is in-line to the left of the button.

J=SLAP-1718
TEST=manual

Check that both professional location cards (and multilang) have correct distance alignment, matching the location-standard card. Check that mobile view of vertical full page map has correct alignment of distance with close card button for location-standard card

@coveralls
Copy link

coveralls commented Nov 15, 2021

Coverage Status

Coverage remained the same at 9.19% when pulling 194d928 on dev/fix-distance-alignment into d4a56a7 on release/v1.26.

Copy link
Contributor

@yen-tt yen-tt left a comment

Choose a reason for hiding this comment

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

instead of changing the templates and adjusting the ordinal and title for all cards, I believe you can just add margin-left: auto to HitchhikerLocationCard-closeCardButtonWrapper for these cards and that should push the distance to far right. This would work for the last element when the -topRow div's display mode is flex

@nmanu1
Copy link
Contributor Author

nmanu1 commented Nov 15, 2021

I was mimicking how this is done for the location-standard card for consistency. But I can change it to this if it's preferred

<div class="HitchhikerFinancialProfessionalLocation HitchhikerLocationCard {{cardName}}">
{{> image }}
<div class="HitchhikerFinancialProfessionalLocation-body">
{{#if (any card.title card.distance)}}
<div class="HitchhikerFinancialProfessionalLocation-topRow">
{{> ordinal }}
{{> title }}
{{> ordinalAndTitle displayOrdinal=(all card.showOrdinal result.ordinal) }}
<div class="HitchhikerLocationCard-closeCardButtonWrapper">
{{> closeCardButton }}
{{> distance }}
Copy link
Contributor

@yen-tt yen-tt Nov 15, 2021

Choose a reason for hiding this comment

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

Should distance be part of closeCardButtonWrapper div? wouldn't this look a bit weird if both distance and closeCardButton is display on the card?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think it would look weird. but I think we would want both of these to be right-aligned. so maybe distance should go before the button and the wrapper should have flex styling?

Copy link
Contributor

@yen-tt yen-tt Nov 15, 2021

Choose a reason for hiding this comment

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

looks like the button is not configurable? seems to have css display: none by default. Perhaps you could checkin with rose if this closecard + distance order needs to be fix. And regard to the distance alignment, maybe she would have an opinion between the template changes to be same as location-standard card, or css styling changes only

Copy link
Member

Choose a reason for hiding this comment

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

Since distance is already part of closeCardButtonWrapper, I think that's out of scope of this QA item. As for the DOM changes, no CSS classes are being removed in this PR which means that most styling that is currently targeting them wouldn't break during an upgrade. So I'd say handling this with CSS only changes isn't critical

@nmanu1 nmanu1 requested a review from cea2aj November 15, 2021 23:36
@nmanu1 nmanu1 requested a review from yen-tt November 16, 2021 00:03
@nmanu1 nmanu1 merged commit ffda2ea into release/v1.26 Nov 16, 2021
@nmanu1 nmanu1 mentioned this pull request Nov 16, 2021
nmanu1 added a commit that referenced this pull request Nov 16, 2021
## Version 1.26.0
### Features
- Add support for the auth token, with a new global_config `token` field  (#976)
- Add support for the visitor, which can be set or changed at runtime (#970)
- Add support for generic thumbs up/down feedback buttons to all cards (#973, #1005)
- Handle page and template names with spaces (#988)

### Changes
- Update styling of navigation ‘More’ menu (#989)
- Update hover color of search bar buttons (#1000)
- Internal repo changes (automate WCAG testing (#987))

### Bugfixes
- Fix console error which would appear on google maps (#955)
- Fix extra query when loading in iframe integrations (#986)
- Fix bug that displayed links that didn’t exist (#990)
- Allow microphone access for voice search in iframe (#991)
- Fix issue with custom search icon size (#992)
- Fix bug with new prop comments not being added (#1004)
- Fix distance alignment on location cards (#1006)
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

4 participants