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: (core) Update popover's usage of side nav, add important files of popover to public_api.ts #1645

Merged
merged 2 commits into from
Jan 6, 2020

Conversation

JKMarkowski
Copy link
Contributor

@JKMarkowski JKMarkowski commented Nov 21, 2019

Please provide a link to the associated issue.

fixes: #1644

Please provide a brief summary of this pull request.

There is new side navigation component usage on popover's last example.
Also during implementation I noticed that there are some files missing in popover's public_api,ts

Please check whether the PR fulfills the following requirements

Documentation checklist:

@JKMarkowski JKMarkowski added the core Core library specific issues label Nov 21, 2019
@JKMarkowski JKMarkowski added this to the sprint 24 - Maester Aemon milestone Nov 21, 2019
@JKMarkowski JKMarkowski added this to In progress in Development via automation Nov 21, 2019
@netlify
Copy link

netlify bot commented Nov 21, 2019

Deploy preview for fundamental-ngx ready!

Built with commit 96b098d

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

@JKMarkowski JKMarkowski changed the title docs: (core) Update usage of side nav, add important files of popover to public_api.ts docs: (core) Update popover's usage of side nav, add important files of popover to public_api.ts Nov 22, 2019
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.

Maybe it needs rebasing but I still see the third level of navigation in the examples. Also, the padding's not correct in the example with only text.
When there's only text, the second level items should have 2rem left padding. The next level adds +1rem padding and so on. In NGX the second level items have 2.5rem left padding.
Screen Shot 2019-11-25 at 2 12 37 PM
Screen Shot 2019-11-25 at 2 10 22 PM

Development automation moved this from In progress to Review in progress Nov 25, 2019
@JKMarkowski JKMarkowski force-pushed the fix/1644-change-popover-example branch from b799612 to 7c33c55 Compare November 26, 2019 08:47
@JKMarkowski JKMarkowski removed the request for review from stefanoScalzo December 2, 2019 10:29
@JKMarkowski
Copy link
Contributor Author

Hi @InnaAtanasova I think this issue occurs, cause of netlify's cache issues. I think we can merge it.

@JKMarkowski JKMarkowski force-pushed the fix/1644-change-popover-example branch 2 times, most recently from 79f8b4c to a86974b Compare December 3, 2019 09:41
@InnaAtanasova
Copy link
Contributor

It still has the issue with the paddings.
NGX:
ngx_1

Styles:
styles_1

Second level items should not have icons. You need to remove the icons from the example:
ngx_2

It should look like this (also the paddings):
styles_2

@JKMarkowski JKMarkowski force-pushed the fix/1644-change-popover-example branch from 8721f88 to 96b098d Compare January 3, 2020 10:03
@JKMarkowski
Copy link
Contributor Author

@InnaAtanasova Changes fixing the padding are included now. I also removed 2nd level icons.

@InnaAtanasova
Copy link
Contributor

The text and the icon are not aligned properly:
Screen Shot 2020-01-03 at 9 55 31 AM

See a comparison with Styles:
Screen Shot 2020-01-03 at 9 54 31 AM

@stefanoScalzo
Copy link
Contributor

The focus is going outside of the side nav
Screen Shot 2020-01-03 at 1 17 03 PM

@JKMarkowski
Copy link
Contributor Author

Hi @InnaAtanasova , @stefanoScalzo I think these comments went too far. This PR was only about fixing side-nav usage on popover component.
@InnaAtanasova For wrong alignment I'm not able to reproduce such a error,
@stefanoScalzo For Focus it's about styles.

Development automation moved this from Review in progress to Reviewer approved Jan 6, 2020
@JKMarkowski JKMarkowski merged commit 538859f into master Jan 6, 2020
Development automation moved this from Reviewer approved to Done Jan 6, 2020
@JKMarkowski JKMarkowski deleted the fix/1644-change-popover-example branch January 6, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core library specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popover example has outdated side navigation.
4 participants