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

fix: (platform) updating button with styles 0.12.0 #3514

Merged
merged 2 commits into from
Oct 2, 2020
Merged

fix: (platform) updating button with styles 0.12.0 #3514

merged 2 commits into from
Oct 2, 2020

Conversation

sKudum
Copy link
Contributor

@sKudum sKudum commented Sep 30, 2020 •
edited

Please provide a link to the associated issue.

#3370

Please provide a brief summary of this pull request.

The [label] property should be used now instead of adding text as a content. The content still works fine, unless it's used with icons.

BREAKING CHANGE:

The value for the text button is now passed as an input property, not as content projection.

Before: <fdp-button>Text</fdp-button>
After: <fdp-button label="Text"></fdp-button>

Please check whether the PR fulfills the following requirements

Documentation checklist:

@netlify
Copy link

netlify bot commented Sep 30, 2020 •
edited

Deploy preview for fundamental-ngx ready!

Built with commit 505e14f

https://deploy-preview-3514--fundamental-ngx.netlify.app

@InnaAtanasova
Copy link
Contributor

InnaAtanasova commented Sep 30, 2020 •
edited

I don't know if this is in the scope of this PR (Core team usually targets this in 1 PR) but here you only address the Button changes in the Button documentation. For example, the buttons in Search field are broken:
Screen Shot 2020-09-30 at 11 10 11 AM
Screen Shot 2020-09-30 at 11 10 03 AM

@sKudum
Copy link
Contributor Author

sKudum commented Sep 30, 2020

I don't know if this is in the scope of this PR (Core team usually targets this in 1 PR) but here you only address the Button changes in the Button documentation. For example, the buttons in Search field are broken:
Screen Shot 2020-09-30 at 11 10 11 AM
Screen Shot 2020-09-30 at 11 10 03 AM

In other components documentation where Buttons are used, the markup is not updated. Here for example, the value of the button is passed through content projection, not as an input prop:
Screen Shot 2020-09-30 at 11 11 52 AM

Hi @InnaAtanasova, Respective component owners will be taking care of any breaking changes.
In this PR only button, we are addressing. @manjunathanagaraj has informed the team already about it.

@InnaAtanasova
Copy link
Contributor

I don't know if this is in the scope of this PR (Core team usually targets this in 1 PR) but here you only address the Button changes in the Button documentation. For example, the buttons in Search field are broken:
Screen Shot 2020-09-30 at 11 10 11 AM
Screen Shot 2020-09-30 at 11 10 03 AM
In other components documentation where Buttons are used, the markup is not updated. Here for example, the value of the button is passed through content projection, not as an input prop:
Screen Shot 2020-09-30 at 11 11 52 AM

Hi @InnaAtanasova, Respective component owners will be taking care of any breaking changes.
In this PR only button, we are addressing. @manjunathanagaraj has informed the team already about it.

Sounds good. I removed the second part of my comment as I saw both ways are possible, but I still think the docs should be unified. It's up to your team to decide.

@InnaAtanasova
Copy link
Contributor

The rest looks good. If your team approve the PR I will add my approval right after. Thanks

KevinOkamoto
KevinOkamoto suggested changes Sep 30, 2020
View reviewed changes
Copy link
Member

@KevinOkamoto KevinOkamoto left a comment

Choose a reason for hiding this comment

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

See comments.

libs/platform/src/lib/components/button/button.component.html Outdated Show resolved Hide resolved
libs/platform/src/lib/components/button/button.component.ts Outdated Show resolved Hide resolved
KevinOkamoto
KevinOkamoto approved these changes Oct 1, 2020
View reviewed changes
Copy link
Member

@KevinOkamoto KevinOkamoto left a comment

Choose a reason for hiding this comment

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

Looks good!

@sKudum
update platform button with 3400 changes
f2fc2fc
@sKudum
removed blank space
505e14f 
updating kevins review commmnts

imported gyphposition from core
@sKudum sKudum merged commit 75a2ea7 into master Oct 2, 2020
@sKudum sKudum deleted the feat/updatebutton branch October 2, 2020 07:00
AntonOlkhovskyi pushed a commit to AntonOlkhovskyi/fundamental-ngx that referenced this pull request Nov 16, 2020
@sKudum
fix: (platform) updating button with styles 0.12.0 (SAP#3514)
217c59e 
* update platform button with 3400 changes

* removed blank space

updating kevins review commmnts

imported gyphposition from core
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@InnaAtanasova InnaAtanasova InnaAtanasova approved these changes

@KevinOkamoto KevinOkamoto KevinOkamoto approved these changes

@fkolar fkolar Awaiting requested review from fkolar

@salarenko salarenko Awaiting requested review from salarenko

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sKudum @InnaAtanasova @KevinOkamoto