Skip to content

Conversation

@oneolddev
Copy link
Contributor

@oneolddev oneolddev 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

@oneolddev oneolddev 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

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

@oneolddev oneolddev 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.

@oneolddev
Copy link
Contributor Author

@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
@oneolddev oneolddev 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.

3 participants