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 wrapper component for info Label with document changes #2486

Merged
merged 8 commits into from Jun 2, 2020
Merged

fix: Platform wrapper component for info Label with document changes #2486

merged 8 commits into from Jun 2, 2020

Conversation

Lokanathan-k
Copy link
Contributor

Please provide a link to the associated issue.

#2338

Please provide a brief summary of this pull request.

This PR is mainly for converting InfoLabel directive from core to Info Label Component in platform
The Info Label component is used to highlight the status of an information by coloring the text.

Please check whether the PR fulfills the following requirements

Documentation checklist:

  • Documentation Examples
  • Stackblitz works for all examples

@Lokanathan-k Lokanathan-k self-assigned this May 8, 2020
@netlify
Copy link

netlify bot commented May 8, 2020 •
edited

Deploy preview for fundamental-ngx ready!

Built with commit 81f7d72

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

@Lokanathan-k
Copy link
Contributor Author

@salarenko i am facing the build issue with the test script can you pls look into it.

Screenshot 2020-05-07 at 10 40 47 PM

@Lokanathan-k Lokanathan-k reopened this May 8, 2020
@fkolar fkolar added this to Approved by Reviewer in Platform Development via automation May 8, 2020
@fkolar fkolar self-requested a review May 8, 2020 07:49
salarenko
salarenko reviewed May 8, 2020
View reviewed changes
Copy link
Contributor

@salarenko salarenko left a comment

Choose a reason for hiding this comment

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

@Lokanathan-k
Thanks for showing me this error. Its going to be fixed with #2487

.../src/app/platform/component-docs/platform-info-label/platform-info-label-docs.component.html Outdated Show resolved Hide resolved
...cs/src/app/platform/component-docs/platform-info-label/platform-info-label-docs.component.ts Outdated Show resolved Hide resolved
libs/platform/src/lib/components/info-label/info-label.component.ts Outdated Show resolved Hide resolved
@Lokanathan-k
Copy link
Contributor Author

@salarenko regarding the Lazy loading comment on the PR 2407 ,can you give more info on that pls where to add the lazy loading

Screenshot 2020-05-11 at 5 27 59 PM

droshev
droshev requested changes May 11, 2020
View reviewed changes
Copy link
Contributor

@droshev droshev left a comment

Choose a reason for hiding this comment

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

@Lokanathan-k I am sorry, man, this title Info Label Platform changes is very generic to me

Platform Development automation moved this from Approved by Reviewer to In Review May 11, 2020
@Lokanathan-k Lokanathan-k changed the title fix: Info Label Platform changes with doc changes fix: Platform Info Label component changes with document changes May 11, 2020
@Lokanathan-k Lokanathan-k requested a review from droshev May 11, 2020 15:47
@Lokanathan-k Lokanathan-k changed the title fix: Platform Info Label component changes with document changes fix: Platform wrapper component for info Label with document changes May 11, 2020
@salarenko
Copy link
Contributor

salarenko commented May 12, 2020 •
edited

Hey @Lokanathan-k, Lazy Loading concept I mentioned is based on dividing application into parts and requesting specified parts of the application in the time when they really need to be used.

At this moment when PlatformDocumentationModule loads it comes with all of the documentation components which aren't really needed. The goal is to separate this components into ComponentDocumentationModules and load them on demand, for example when application navigates to specified URL. I recommend reading the documentation to get more familiar with this concept in Angular.

@Lokanathan-k
Copy link
Contributor Author

the documentation

Thanks for the confirmation of this. will address this change in a seperate PR, we will make a not of this this and work acordingly for this change.

JKMarkowski
JKMarkowski reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@JKMarkowski JKMarkowski left a comment

Choose a reason for hiding this comment

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

LGTM, only one minor comment

@Lokanathan-k
Copy link
Contributor Author

Lokanathan-k commented May 19, 2020 •
edited

Loki, when you commit new things into existing PR it needs again another cycle of approval. Why did you push this into existing PR?

@fkolar it was not any code change, It was a conflict resolving, Post the split button changes was affecting my code, this conflict resolving was nessary, and few test failed so this change was needed

droshev
droshev requested changes May 20, 2020
View reviewed changes
Copy link
Contributor

@droshev droshev left a comment

Choose a reason for hiding this comment

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

@Lokanathan-k I see that you copied the documentation from the core package, which would raise the question: why and when the developers should the platform InfoLabel instead of the core one. It has to be very clear in which use cases we recommend that component from the platform.

@fkolar fkolar self-requested a review May 20, 2020 16:03
@Lokanathan-k
Copy link
Contributor Author

Lokanathan-k commented May 20, 2020 •
edited

@Lokanathan-k I see that you copied the documentation from the core package, which would raise the question: why and when the developers should the platform InfoLabel instead of the core one. It has to be very clear in which use cases we recommend that component from the platform.

@droshev i have added the changes in platform features in the documentation part.

@Lokanathan-k Lokanathan-k requested a review from droshev May 20, 2020 17:35
droshev
droshev requested changes May 20, 2020
View reviewed changes
Copy link
Contributor

@droshev droshev left a comment

Choose a reason for hiding this comment

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

You wrote:
- Info Label offers additional option like Aria attributes for Accessibility.
do u have an example or more details how they can be used?
Can you give an example where it makes more sense to use the platform component instead of the one from core

@Lokanathan-k Lokanathan-k requested a review from droshev May 27, 2020 16:54
@Lokanathan-k
Copy link
Contributor Author

You wrote:
- Info Label offers additional option like Aria attributes for Accessibility.
do u have an example or more details how they can be used?
Can you give an example where it makes more sense to use the platform component instead of the one from core

Added @droshev @fkolar can you pls review and approve pls

droshev
droshev reviewed May 28, 2020
View reviewed changes
Copy link
Contributor

@droshev droshev left a comment

Choose a reason for hiding this comment

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

Looks fine to me too, if you can order the components in alphabetical order would be great!
thanks

apps/docs/src/app/platform/documentation/platform-documentation.component.ts Outdated Show resolved Hide resolved
@Lokanathan-k
Code Commit for the Info label platform component
59e0631 
Code Commit for the Info label platform component

Few document change s for the info lable component

Few document change s for the info lable component

Update info-label.component.ts

updating the test case for the color change

updating the test case for the color change

remove the constructor

remove the constructor

Updating the docuement loading changes

Updating the docuement loading changes
@Lokanathan-k
adding lazy loadin to the code
6b85c76 
adding lazy loadin to the code
@Lokanathan-k
document changes for the info label Lazy loading
7648d54 
document changes for the info label Lazy loading
@Lokanathan-k
Update info-label.component.spec.ts
036690d
@Lokanathan-k
updating the documentation and accessability attributes
d327806 
updating the documentation and accessability attributes
@Lokanathan-k
adding aria attributes for accessability
d5e1ebc 
adding aria attributes for accessability
@Lokanathan-k
Update platform-info-label-header.component.html
56ab67f
@Lokanathan-k
Update platform-documentation.component.ts
81f7d72 
Reordering the URL  code alpahabeticallly
@Lokanathan-k Lokanathan-k requested a review from droshev May 29, 2020 03:58
@droshev droshev added the platform platform label Jun 2, 2020
Platform Development automation moved this from In Review to Approved by Reviewer Jun 2, 2020
@Lokanathan-k Lokanathan-k merged commit de981b0 into master Jun 2, 2020
@JKMarkowski JKMarkowski deleted the platform/infoLabel branch June 5, 2020 07:39
fkolar pushed a commit that referenced this pull request Jun 5, 2020
@Lokanathan-k @fkolar
fix: Platform wrapper component for info Label with document changes (#…
9eddd0a 
…2486)

* Code Commit for the Info label platform component

Code Commit for the Info label platform component

Few document change s for the info lable component

Few document change s for the info lable component

Update info-label.component.ts

updating the test case for the color change

updating the test case for the color change

remove the constructor

remove the constructor

Updating the docuement loading changes

Updating the docuement loading changes

* adding lazy loadin to the code

adding lazy loadin to the code

* document changes for the info label Lazy loading

document changes for the info label Lazy loading

* Update info-label.component.spec.ts

* updating the documentation and accessability attributes

updating the documentation and accessability attributes

* adding aria attributes for accessability

adding aria attributes for accessability

* Update platform-info-label-header.component.html

* Update platform-documentation.component.ts

Reordering the URL  code alpahabeticallly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droshev droshev droshev approved these changes

@rengare rengare rengare approved these changes

@fkolar fkolar fkolar approved these changes

@salarenko salarenko salarenko approved these changes

@JKMarkowski JKMarkowski JKMarkowski approved these changes

@KevinOkamoto KevinOkamoto KevinOkamoto approved these changes

@InnaAtanasova InnaAtanasova Awaiting requested review from InnaAtanasova

Assignees

@Lokanathan-k Lokanathan-k

Labels
platform platform
Projects
No open projects
Platform Development
  
Approved by Reviewer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Lokanathan-k @salarenko @droshev @fkolar @rengare @JKMarkowski @KevinOkamoto