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

chore(NcAppNavigationToggle): add comments #5148

Merged
merged 1 commit into from Jan 29, 2024
Merged

chore(NcAppNavigationToggle): add comments #5148

merged 1 commit into from Jan 29, 2024

Conversation

emoral435
Copy link
Contributor

☑️ For nextcloud/photos#2278

🖼️ Screenshots

🏚️ Before 🏡 After
firefox_lHUQ9YnsiM firefox_mBytSzpcFp

🚧 Tasks

This has the same changes from nextcloud/photos#2285

  • When the viewport got small in the photos app, the app-navigation-button overflowed to the right of the screen, becoming inaccessible
  • To fix this, since the button is positioned relative to the header, I removed some of the negative right positioning on the button

This allows the button to still be in view, while a little tight, and also still allows it to be found with keyboard navigation 👍
Any styling choices can be commented on !

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@emoral435 emoral435 added enhancement New feature or request 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Jan 26, 2024
@emoral435 emoral435 added this to the 8.5.1 milestone Jan 26, 2024
@emoral435 emoral435 self-assigned this Jan 26, 2024
@emoral435 emoral435 force-pushed the fix/a11y/2278/not-possible-to-close-left-sidebar-on-small-screens branch from 96a1f06 to 94071de Compare January 26, 2024 15:27
@emoral435
Copy link
Contributor Author

Update: added some comments as well for the docs 👍

@emoral435 emoral435 force-pushed the fix/a11y/2278/not-possible-to-close-left-sidebar-on-small-screens branch from 94071de to 753f5c2 Compare January 26, 2024 15:31
susnux
susnux requested changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

I do not think this belongs here but to the photos app, they mess with the styles so it needs to be fixed there.
There are really not nice hacks for the app navigation and event this "force position" is not fixing all problems in photos.

But the doc changes are nice!

@susnux susnux mentioned this pull request Jan 26, 2024
Merged
@emoral435 emoral435 closed this Jan 26, 2024
@emoral435 emoral435 removed this from the 8.5.1 milestone Jan 26, 2024
@emoral435 emoral435 reopened this Jan 26, 2024
@emoral435 emoral435 added this to the 8.5.1 milestone Jan 26, 2024
@emoral435
Copy link
Contributor Author

Agreed ! Closed the PR, but decided to reopen it for the docs, but not the most important PR ever so this can always wait!

@emoral435 emoral435 force-pushed the fix/a11y/2278/not-possible-to-close-left-sidebar-on-small-screens branch 3 times, most recently from cc41a21 to cb4275d Compare January 26, 2024 17:29
ShGKme
ShGKme reviewed Jan 26, 2024
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

This component is supposed to be used only in <NcAppNavigation> and actually is not exported from the library.

So we should remove it from the docs at all instead. (in the styleguidist config)

src/components/NcAppNavigationToggle/NcAppNavigationToggle.vue Outdated
Comment on lines 48 to 52
/**
* The NcAppNavigationToggle component is used typically alongside another header navigation component.
* It emits the status of whether the button has been clicked or not.
*
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

It is supposed to be used only in <NcAppNavigation> and is not exported from the library.

We should probably remove it from the docs instead.

@susnux
Copy link
Contributor

susnux commented Jan 26, 2024

But I would keep the doc for internal purpose to understand the component itself.
We do not need to publish in styleguide, but keep for us.(?)

@ShGKme
Copy link
Contributor

ShGKme commented Jan 26, 2024

but keep for us.(?)

Not sure so detailed documentation is needed internally for so small a component and props like open for a toggle button.

@susnux susnux modified the milestones: 8.5.1, 8.6.0 Jan 26, 2024
@emoral435
enh: added comments on NcHeaderMenu for NcVue documentation
1d3a181 
Signed-off-by: Eduardo Morales <emoral435@gmail.com>
@emoral435 emoral435 force-pushed the fix/a11y/2278/not-possible-to-close-left-sidebar-on-small-screens branch from cb4275d to 1d3a181 Compare January 29, 2024 13:10
@emoral435
Copy link
Contributor Author

Removed the comment @ShGKme mentioned, as I agree with him 🙏 Further in the future, if it does get used for more things, it can be added again, so I am indifferent 👍 small PR now 😭

@susnux susnux merged commit 7665b6e into master Jan 29, 2024
15 checks passed
@susnux susnux deleted the fix/a11y/2278/not-possible-to-close-left-sidebar-on-small-screens branch January 29, 2024 17:58
@ShGKme ShGKme removed enhancement New feature or request accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Jan 29, 2024
@ShGKme ShGKme changed the title fix: app-navigation-toggle remains on screen on small viewports chore(NcAppNavigationToggle): add comments Jan 29, 2024
@Antreesy Antreesy mentioned this pull request Jan 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susnux susnux susnux approved these changes

@JuliaKirschenheuter JuliaKirschenheuter JuliaKirschenheuter approved these changes

@Pytal Pytal Awaiting requested review from Pytal

@ShGKme ShGKme Awaiting requested review from ShGKme

Assignees

@emoral435 emoral435

Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
8.6.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@emoral435 @susnux @ShGKme @JuliaKirschenheuter