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

Feature: Removed margin when using the Compact Spacing option #13145

Merged
merged 5 commits into from
Aug 7, 2023

Conversation

yaira2
Copy link
Member

@yaira2 yaira2 commented Aug 6, 2023

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Feature: Reduce margin in the compact spacing option #12012...

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Enable compact spacing
    2. Confirm there is no space between items in the details layout

Screenshots (optional)
image

@yaira2 yaira2 changed the title Feature: Remove margin when using the Compact Spacing option Feature: Removed margin when using the Compact Spacing option Aug 6, 2023
@mdtauk
Copy link
Contributor

mdtauk commented Aug 7, 2023

This looks poorly designed

@ferrariofilippo
Copy link
Contributor

This looks poorly designed

Actually I love it
Anyway, @yaira2 I get this exception when I try to navigate to Home. The issue is that I fall in a crash loop and I can't start the app again
image

@mdtauk
Copy link
Contributor

mdtauk commented Aug 7, 2023

I understand people wanting closer spacing, but it still needs some tweaks to make it look polished

@yaira2
Copy link
Member Author

yaira2 commented Aug 7, 2023

@ferrariofilippo can you check if the latest commit fixed the exception?

@ferrariofilippo
Copy link
Contributor

Everything works fine now! I have another request: can you add VerticalAlignement = Center to RecentFiles, SecurityPage and AdvancedSecurityPage?

@yaira2
Copy link
Member Author

yaira2 commented Aug 7, 2023

SecurityPage and AdvancedSecurityPage?

Do these need it? It should be using the default template now (compact styles don't apply to these pages anymore).

RecentFiles

It's already applied to recent files, can you check again?

@ferrariofilippo
Copy link
Contributor

SecurityPage and AdvancedSecurityPage?

Do these need it? It should be using the default template now (compact styles don't apply to these pages anymore).

I don't know why but I could still see the compact layout. Now it looks good!

RecentFiles

It's already applied to recent files, can you check again?

It's working. I guess .txt icons have some spacing above, since they are the only ones who doesn't seem centered

@yaira2
Copy link
Member Author

yaira2 commented Aug 7, 2023

Can you share a screenshot?

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

LGTM!

@ferrariofilippo
Copy link
Contributor

Can you share a screenshot?

2
1

The txt file has 1/2 pixels of space above, instead the js file fills all the space

@yaira2
Copy link
Member Author

yaira2 commented Aug 7, 2023

This will be fixed when we switch to using smaller icons with compact spacing.

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Aug 7, 2023
@yaira2 yaira2 merged commit b03c2fd into main Aug 7, 2023
@yaira2 yaira2 deleted the ya/CompactSpacing branch August 7, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Reduce margin in the compact spacing option
3 participants