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.

Sign up for GitHub

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

Jump to bottom

legacy-v4 [on hold] #311

Closed
wants to merge 13 commits into from
Closed

legacy-v4 [on hold] #311

wants to merge 13 commits into from

Conversation

dpikt
Copy link
Contributor

@dpikt dpikt commented Jan 9, 2019 •
edited
Loading

Rachel Killackey added 3 commits April 17, 2018 13:52
Add hideLabel prop (#227)
6ab042f 
* add hideLabel prop, story

* update showLabel logic, add new story

* remove import

* Version bump: 3.7.0

* add input label test, fix range input test

* lint

* change range label prop name
merge master
3efd628
Add Select placeholder option by default (#256)
3b00e6e 
* add default placeholder

* Version bump: 3.12.0
@dpikt dpikt had a problem deploying to lp-components-pr-311 January 9, 2019 21:27 Failure
@chawes13
Copy link
Contributor

@dpikt Do you want to merge latest in here for me to use this as the base for my PR?

@dpikt
Copy link
Contributor Author

dpikt commented Apr 19, 2019

@chawes13 yes plz!

@chawes13
Copy link
Contributor

@dpikt Do you still want to keep hideLabel? I'm not sure that I understand the benefits?

@dpikt
Copy link
Contributor Author

dpikt commented Apr 19, 2019

@chawes13 I think the idea was that passing label={false} was confusing. But since we're very used to it now I'm not sure if there's a big advantage to switching it up.

@chawes13
Copy link
Contributor

Yeah I think that's fair and it is confusing. However, if we're going to introduce it in v4, shouldn't we strip out the backwards compatibility?

@dpikt
Copy link
Contributor Author

dpikt commented Apr 19, 2019

@chawes13 up to you. It's technically in v4 because of the change to RangeInput. Personally I'd like to keep the required changes to a minimum and just adapt our best practices. But it's also a pretty easy change (find/replace label={ false })

@chawes13
Copy link
Contributor

@dpikt I took it out. I also fixed some linting errors in 66282a5 but didn't create a separate PR for that. LMK if any of the changes are a concern.

@dpikt
Copy link
Contributor Author

dpikt commented Apr 19, 2019

Fine by me as long as you remember to put it in the migration guide 👍

@chawes13
Copy link
Contributor

Well, technically we don't have to mention it in the migration guide since it's not a change. I'll include the bit about RangeInput though

@dpikt
Copy link
Contributor Author

dpikt commented Aug 23, 2019

@chawes13 can you remind me what the blockers are (if any) to merging this?

@chawes13
Copy link
Contributor

@dpikt I believe none!

@dpikt
Copy link
Contributor Author

dpikt commented Aug 23, 2019

😲

@dpikt
Copy link
Contributor Author

dpikt commented Aug 23, 2019

I don't see a migration guide... 😏

@chawes13
Copy link
Contributor

Hm, I don't have time today but can tackle it next week.

@dpikt
Copy link
Contributor Author

dpikt commented Aug 23, 2019

Sweet 👌 a good first step might be to add the resolved issues to the description.

@chawes13
Copy link
Contributor

chawes13 commented Oct 9, 2019

@dpikt ...forgot about this

chawes13 and others added 6 commits October 10, 2019 06:41
@chawes13
a11y group legends (#384)
513ed55 
* Default label component to a legend for checkbox group

* Default label component to a legend for radio group
@dpikt
Merge branch 'master' into v4
b57e13a 
# Conflicts:
#	docs.md
#	package.json
#	src/forms/buttons/button.js
#	test/forms/inputs/select.test.js
@dpikt
Remove custom style prop on button (#408)
6648ee0 
* Remove custom style prop on button

* Remove disabled button test

* Cleanup

* Some formatting

* Priority -> variant
@dpikt
Merge
caefffd
@dpikt
Add migration guide
0ce4fca
@dpikt
Naming
813a0bf
@dpikt
Copy link
Contributor Author

dpikt commented Jan 31, 2020

@chawes13 can you review the migration guide? https://github.com/LaunchPadLab/lp-components/blob/79a0490524cd112850c88945daa18b9d9e915c3f/migration-guides/v4.0.0.md

@dpikt dpikt requested a review from chawes13 January 31, 2020 20:08
@chawes13 chawes13 mentioned this pull request Feb 6, 2020
Merged
@dpikt dpikt mentioned this pull request Jul 17, 2020
Open
@dpikt dpikt changed the title v4 v4 [on hold] Sep 1, 2020
@dpikt dpikt changed the title v4 [on hold] legacy-v4 [on hold] Sep 1, 2020
@chawes13 chawes13 removed their request for review September 4, 2020 16:24
@chawes13 chawes13 marked this pull request as draft August 31, 2021 16:17
@chawes13 chawes13 removed their assignment Aug 31, 2021
@chawes13 chawes13 closed this Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dpikt @chawes13