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

[FluentIcon] Minor cleanup and clarification #2038

Merged
merged 6 commits into from May 18, 2024
Merged

Conversation

c0g1t8
Copy link
Contributor

@c0g1t8 c0g1t8 commented May 12, 2024

Pull Request

πŸ“– Description

  • Minor cleanup on code rendered for FluentIcon. Removes invalid width attribute from SVG style when Width set to string.Empty
  • Update documentation to clarify behavior when Width set to string.Empty.

🎫 Issues

For responsive layout, it is preferable to use CSS versus embedding the style in HTML. The width of the SVG is embedded unless FluentIcon's Width is explicity set to string.Empty

πŸ‘©β€πŸ’» Reviewer Notes

Any suggestions to the update on the documentation page? I wanted to add some information about this behavior without getting into a specific use case.

πŸ“‘ Test Plan

βœ… Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new compontent
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

@c0g1t8 c0g1t8 marked this pull request as ready for review May 12, 2024 23:33
Copy link
Collaborator

@dvoituron dvoituron left a comment

Choose a reason for hiding this comment

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

Could

examples/Demo/Shared/Pages/Icon/IconPage.razor Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvoituron dvoituron left a comment

Choose a reason for hiding this comment

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

Could you add an Unit Test to validate this new feature?

@vnbaaij vnbaaij added this to the v4.7.3 milestone May 15, 2024
@vnbaaij vnbaaij added chore Maintenance or non-code work improvement A non-feature-adding improvement labels May 15, 2024
@vnbaaij vnbaaij dismissed their stale review May 16, 2024 07:26

Invalid solution suggested

@c0g1t8 c0g1t8 requested a review from dvoituron May 17, 2024 06:28
@dvoituron
Copy link
Collaborator

dvoituron commented May 17, 2024

Weird... Your PR doesn't seem to accept the latest dev changes.
It currently contains 41 file changes when in fact there are only 4.

image

image

Could you check this anomaly (perhaps to close this PR and recreate a new one... I don't know)?

About the code, everything seems to be correct.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented May 17, 2024

@dvoituron - I'll clean up and resubmit.

@vnbaaij vnbaaij enabled auto-merge (squash) May 18, 2024 08:09
@vnbaaij vnbaaij merged commit f59d054 into microsoft:dev May 18, 2024
2 checks passed
@c0g1t8 c0g1t8 deleted the FluentIcon branch May 18, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintenance or non-code work improvement A non-feature-adding improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants