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

feat(NcButton): Add pressed state for stateful buttons #4344

Merged
merged 3 commits into from Jul 19, 2023
Merged

feat(NcButton): Add pressed state for stateful buttons #4344

merged 3 commits into from Jul 19, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jul 18, 2023 •
edited

☑️ Resolves

This simply adds a pressed state and enforces primary on pressed, and any other type (except primary) otherwise.
Not we should probably remove the button variant of the checkboxradioswitch for not grouped elements to prevent different styled components with the same function.

🖼️ Screenshots

vokoscreenNG-2023-07-18_12-28-37.mp4

🏁 Checklist

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

@susnux susnux added 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: button labels Jul 18, 2023
@susnux susnux force-pushed the fix/ncbutton-pressed-style branch from 56eed6a to 1e1eac9 Compare July 18, 2023 18:20
@susnux susnux requested a review from ShGKme July 18, 2023 18:20
@susnux susnux force-pushed the fix/ncbutton-pressed-style branch from 1e1eac9 to b4b50a0 Compare July 18, 2023 19:11
ShGKme
ShGKme reviewed Jul 18, 2023
View reviewed changes
src/components/NcButton/NcButton.vue Outdated Show resolved Hide resolved
src/components/NcButton/NcButton.vue Outdated Show resolved Hide resolved
src/components/NcButton/NcButton.vue Outdated Show resolved Hide resolved
@susnux susnux force-pushed the fix/ncbutton-pressed-style branch 3 times, most recently from f4d0376 to c91ef84 Compare July 18, 2023 21:03
@susnux susnux requested a review from ShGKme July 18, 2023 21:04
marcoambrosini
marcoambrosini approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Nice work, thank you :)

JuliaKirschenheuter
JuliaKirschenheuter approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

haven't tested but looks good

ShGKme
ShGKme approved these changes Jul 19, 2023
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.

Tested, works fine, looks good by code

@ShGKme
Copy link
Contributor

ShGKme commented Jul 19, 2023

Cypress tests seems to be related

jancborchardt
jancborchardt approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Nice, looks good! :)

@susnux
Copy link
Contributor Author

susnux commented Jul 19, 2023

Cypress tests seems to be related

Yes as this is a visual change, the snapshots need to be updated, but the other tests pass :)

@susnux susnux force-pushed the fix/ncbutton-pressed-style branch 2 times, most recently from dc27727 to 7aac835 Compare July 19, 2023 16:52
@susnux
Copy link
Contributor Author

susnux commented Jul 19, 2023

Oh yes, I love that cypress on my system renders the snapshots differently than on CI (antialiasing)...

But now the tests are still flaky. As the cursor within the input "flashes" and that is causing the snapshots to differ (compare runner 1 and runner 2)

image

@susnux
feat(NcButton): Add pressed state for stateful buttons, like the `N…
a7d9687 
…cAppSidebar` star button

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/ncbutton-pressed-style branch from 7aac835 to f70d0bf Compare July 19, 2023 17:33
@susnux
fix(cypress): Prevent flaky visual tests by making cursor transparent…
7f9f5ee 
… for snapshot

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
chore: Update cypress snapshots
90400c1 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the fix/ncbutton-pressed-style branch from f70d0bf to 90400c1 Compare July 19, 2023 17:35
@susnux susnux merged commit 04945bc into master Jul 19, 2023
17 checks passed
@susnux susnux deleted the fix/ncbutton-pressed-style branch July 19, 2023 19:50
@susnux susnux mentioned this pull request Jul 19, 2023
Closed
2 tasks
@juliushaertl juliushaertl mentioned this pull request Jul 21, 2023
Closed
1 task
@skjnldsv skjnldsv mentioned this pull request Aug 11, 2023
Merged
@susnux susnux mentioned this pull request Nov 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jancborchardt jancborchardt jancborchardt approved these changes

@JuliaKirschenheuter JuliaKirschenheuter JuliaKirschenheuter approved these changes

@ShGKme ShGKme ShGKme approved these changes

@marcoambrosini marcoambrosini marcoambrosini approved these changes

@raimund-schluessler raimund-schluessler Awaiting requested review from raimund-schluessler

Assignees
No one assigned
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities feature: button
Projects
None yet
Milestone
No milestone
5 participants
@susnux @ShGKme @jancborchardt @JuliaKirschenheuter @marcoambrosini