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

docs: table checkbox #1975

Merged
merged 6 commits into from Feb 17, 2020
Merged

docs: table checkbox #1975

merged 6 commits into from Feb 17, 2020

Conversation

stefanoScalzo
Copy link
Contributor

@stefanoScalzo stefanoScalzo commented Feb 14, 2020

Please provide a link to the associated issue.

defect hunting

Please provide a brief summary of this pull request.

Put the new implementation of checkboxes on table

Please check whether the PR fulfills the following requirements

Documentation checklist:

@stefanoScalzo stefanoScalzo requested a review from a team February 14, 2020 16:37
@stefanoScalzo stefanoScalzo changed the title Fix/table checkbox fix: table checkbox Feb 14, 2020
@netlify
Copy link

netlify bot commented Feb 14, 2020

Deploy preview for fundamental-ngx ready!

Built with commit 967e35d

https://deploy-preview-1975--fundamental-ngx.netlify.com

@stefanoScalzo stefanoScalzo added this to the Sprint 30 - Bangkok milestone Feb 14, 2020
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

  1. Not the correct prefix for the PR title
  2. Could you use compact modifier for the checkboxes? They look huge (the default is cosy which is used in mobile devices)
  3. Please add Project and labels to the PR
  4. Please remove the lines of code that were changed because of reformatting of the code. Thanks

@stefanoScalzo stefanoScalzo changed the title fix: table checkbox docs: table checkbox Feb 14, 2020
@stefanoScalzo stefanoScalzo added this to In progress in Development via automation Feb 14, 2020
@stefanoScalzo stefanoScalzo added the bug Something isn't working label Feb 14, 2020
@InnaAtanasova
Copy link
Contributor

Another problem is Selected rows example in the documentation. When you select a certain row it's number should appear in the Selected rows array. It only works for Select All checkbox.
Before (master):
Screen Shot 2020-02-14 at 1 29 34 PM

Now:
Screen Shot 2020-02-14 at 1 29 56 PM

this.selectedRows.push(row);
console.log(this.tableRows);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the console.log

Development automation moved this from In progress to Review in progress Feb 15, 2020
Copy link
Contributor

@rengare rengare left a comment

Choose a reason for hiding this comment

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

comments have to be addressed

@droshev
Copy link
Contributor

droshev commented Feb 16, 2020

Another problem is Selected rows example in the documentation. When you select a certain row it's number should appear in the Selected rows array. It only works for Select All checkbox.
Before (master):
Screen Shot 2020-02-14 at 1 29 34 PM

Now:
Screen Shot 2020-02-14 at 1 29 56 PM

updated
Screen Shot 2020-02-16 at 4 35 16 PM

@droshev
Copy link
Contributor

droshev commented Feb 16, 2020

@InnaAtanasova @rengare can you review the PR once more, please?

Development automation moved this from Review in progress to Reviewer approved Feb 17, 2020
@droshev droshev self-requested a review February 17, 2020 04:18
@droshev droshev merged commit dc9fb9d into master Feb 17, 2020
Development automation moved this from Reviewer approved to Done Feb 17, 2020
@droshev droshev deleted the fix/table-checkbox branch February 17, 2020 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants