-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Play/1132 fix margin bottom revert #3280
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasperfurniss
approved these changes
Mar 25, 2024
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 6, 2024
**What does this PR do?** A clear and concise description with your runway ticket url. [PBNTR-479](https://runway.powerhrg.com/backlog_items/PBNTR-479) exposes the margin bottom on the text input that lies within a wrapper inside the Typeahead kit so that a dev can to modify kit's `margin_bottom`. The doc examples showcase `margin_bottom` values of: none, xxs, xs, sm, md, lg, and xl. The Rails doc example show a react-rendered rails version of the kit but functions the same in the rails-only version as well. The initial doc example showed both rails versions within the doc example but that felt like overkill. This work is modeled after work previously done on the [datepicker kit here](#3280), and functions the same. There was an additional incidental finding discovered here: the dark margin bottom doc examples for both typeahead (rails and react) and datepicker (rails) did not initially work as expected due to the margin_bottom still being hardcoded into the dark mode section of `text_input.scss`. All doc examples where the prescribed margin_bottom was small or smaller (none through sm) had margin_bottom values that calculated to sm due to the hardcoded value on the text-input-wrapper. The larger ones visually appeared right but upon inspection one can see the additional margin on the text-input-wrapper. This is fixed in this PR (see datepicker screenshots in screenshot section). [PBNTR-373](https://runway.powerhrg.com/backlog_items/PBNTR-479)'s doc examples will require the work from this ticket as this previously-unreachable internal margin prevents the ability to vertically align the child Typeahead kit with the Radio button. **Screenshots:** Screenshots to visualize your addition/change Rails margin bottom doc example <img width="1327" alt="for PR rails doc ex" src="https://github.com/user-attachments/assets/9f285360-2140-4c2b-adb5-25efa9311b5f"> React margin bottom doc example <img width="1324" alt="for PR react dark mode typeahead doc ex" src="https://github.com/user-attachments/assets/178e8f92-f36f-4088-a383-16b0cfb94b43"> Incidental dark mode issue shown with datepicker: margin bottom not working as expected in prod (see first three examples all with margin_bottom small) <img width="1319" alt="dark mode margin bottom wrong datepicker prod" src="https://github.com/user-attachments/assets/e4a42e69-a4b5-4b14-b3e4-0b02ea65e9a5"> margin bottom fixed (all margin bottoms correctly responsive) <img width="1479" alt="dark mode fix datepicker kit" src="https://github.com/user-attachments/assets/6e09cc02-b54a-4f58-9a87-a5e776602811"> **How to test?** Steps to confirm the desired behavior: 1. Go to margin bottom doc example ([rails](https://pr3654.playbook.beta.px.powerapp.cloud/kits/typeahead#margin-bottom)/[react](https://pr3654.playbook.beta.px.powerapp.cloud/kits/typeahead/react#margin-bottom)). 2. Inspect each typeahead to see margin_bottom prop in action. 3. Test typeaheads/typical paths in [nitro-web milano environment](https://pr42318.nitro-web.beta.gm.powerapp.cloud/) to ensure works. #### Checklist: - [x] **LABELS** Add a label: `enhancement`, `bug`, `improvement`, `new kit`, `deprecated`, or `breaking`. See [Changelog & Labels](https://github.com/powerhome/playbook/wiki/Changelog-&-Labels) for details. - [x] **DEPLOY** I have added the `milano` label to show I'm ready for a review. - [x] **TESTS** I have added test coverage to my code.
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do? A clear and concise description with your runway ticket url.
Fixes the revert
5bbc0b5
For the issue created in this PR
#3229