Skip to content

feat(alert):adding alert multi line input#16851

Merged
liamdebeasi merged 18 commits intoionic-team:masterfrom
tawfiek:textarea-in-alert
Nov 11, 2019
Merged

feat(alert):adding alert multi line input#16851
liamdebeasi merged 18 commits intoionic-team:masterfrom
tawfiek:textarea-in-alert

Conversation

@tawfiek
Copy link
Contributor

@tawfiek tawfiek commented Dec 21, 2018

Short description of what this resolves:

Adds the possibility to add a textarea in ion alert.

Changes proposed in this pull request:

  • Android
    screenshot from 2018-12-21 08-19-15

  • IOS
    screenshot from 2018-12-21 08-22-57

Version: 4.0.0-beta.19

Fixes: #14153

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Dec 21, 2018
@tawfiek tawfiek changed the title Adding alert multi line input fix(alert):adding alert multi line input Dec 21, 2018
@tawfiek tawfiek changed the title fix(alert):adding alert multi line input feat(alert):adding alert multi line input Dec 21, 2018
@robingenz
Copy link
Contributor

When is this getting merged?

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

I think there needs to be a discussion as to what the final API should look like, but looks good so far! Left a few comments below.

Are there any other props for textarea that we should support? Maybe @brandyscarney has some ideas here. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea

@liamdebeasi
Copy link
Contributor

Also, can you fix the merge conflicts? Thanks!

@tawfiek
Copy link
Contributor Author

tawfiek commented Mar 31, 2019

I plan to work on these changes at the weekend.
@robingenz @liamdebeasi

tawfiek added 4 commits April 4, 2019 13:55
A case where the user can resize a textarea to be so small the placeholder gets cut off
A case where the user can resize horizontally and then the textarea is partially out of view in the alert.
@tawfiek
Copy link
Contributor Author

tawfiek commented Apr 5, 2019

Done.
any other comments ?!
@liamdebeasi

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Hey there,

Sorry for the delay in reviewing. This is looking great! Could you add another test to basic/index.html?

Also, the resize: vertical doesn't seem to be working. I was thinking of just getting rid of resize altogether since ion-textarea doesn't have it. Thoughts @brandyscarney?

@brandyscarney
Copy link
Member

@liamdebeasi That sounds good.

I think this addition is misleading:

ion-input,prop,type,"date" | "email" | "number" | "password" | "search" | "tel" | "text" | "textarea" | "time" | "url",'text',false,false

ion-input won't allow a textarea to be passed as a type.

@tawfiek
Copy link
Contributor Author

tawfiek commented May 17, 2019

@liamdebeasi That sounds good.

I think this addition is misleading:

ion-input,prop,type,"date" | "email" | "number" | "password" | "search" | "tel" | "text" | "textarea" | "time" | "url",'text',false,false

ion-input won't allow a textarea to be passed as a type.

I thought about this
Any Ideas make it less misleading
@brandyscarney

@tawfiek
Copy link
Contributor Author

tawfiek commented Jun 26, 2019

any updates here !

@robingenz
Copy link
Contributor

Any updates?

Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Besides testing out the UI, I think this is fine to add once merge conflicts and comments are addressed. 🙂

@brandyscarney
Copy link
Member

The code updates look good! We just need to test out the UI and then the merge conflicts need to be resolved. I'll add this to our needs review!

@brandyscarney
Copy link
Member

@tawfiek Is there any way you can fix the merge conflicts on this? I attempted to but it wouldn't let me push to your remote.

@brandyscarney
Copy link
Member

UI looks good. 👍 Just need to fix the merge conflicts.

Merging master of ionic team.
# Conflicts:
#	core/src/components/alert/alert.tsx
#	core/src/components/alert/readme.md
#	core/src/components/alert/test/preview/index.html
#	core/src/components/alert/usage/javascript.md
#	core/src/components/input/readme.md
@tawfiek
Copy link
Contributor Author

tawfiek commented Nov 11, 2019

I am done, but the CI builds fail in test-core-clean-build honestly I have no idea why this build is failing.
@brandyscarney.

@liamdebeasi
Copy link
Contributor

@tawfiek You need to run npm run build from the core directory and then commit/push those changes.

@tawfiek tawfiek requested a review from liamdebeasi November 11, 2019 17:25
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Can you also add a test for this? You can just add the following to alert/test/basic/index.html:134

{
  type: 'textarea',
  placeholder: 'Placeholder 4',
  value: 'Textarea hello'
},

After that we should be good to merge.

@tawfiek tawfiek requested a review from liamdebeasi November 11, 2019 19:05
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Great work!

@brandyscarney
Copy link
Member

@tawfiek Thanks so much for your patience and working with us on this. 🎉

@liamdebeasi liamdebeasi merged commit b28cf02 into ionic-team:master Nov 11, 2019
@tawfiek tawfiek deleted the textarea-in-alert branch November 17, 2019 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants