Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Dialog): add headerAction slot #1617

Merged
merged 14 commits into from
Jul 12, 2019
Merged

Conversation

mnajdova
Copy link
Contributor

This PR adds a headerAction to the Dialog component. This button is text and iconOnly and is rendered after the header slot. I noticed that it is the first thing focused when the Dialog is opened (not sure if this is the behavior we want) @jurokapsiar @sophieH29 please share your thoughts on this. Also the focus indicator is not implemented for the textOnly variant for the button. I may add that as part of this PR.

@vercel vercel bot temporarily deployed to staging July 11, 2019 13:58 Inactive
@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #1617 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1617      +/-   ##
==========================================
- Coverage   71.48%   71.46%   -0.03%     
==========================================
  Files         845      845              
  Lines        6943     6945       +2     
  Branches     1980     1981       +1     
==========================================
  Hits         4963     4963              
- Misses       1974     1976       +2     
  Partials        6        6
Impacted Files Coverage Δ
packages/react/src/components/Dialog/Dialog.tsx 31.57% <ø> (ø) ⬆️
.../themes/teams/components/Dialog/dialogVariables.ts 0% <ø> (ø) ⬆️
.../src/themes/base/components/Dialog/dialogStyles.ts 0% <ø> (ø) ⬆️
...src/themes/teams/components/Button/buttonStyles.ts 3.44% <0%> (-0.13%) ⬇️
...src/themes/teams/components/Dialog/dialogStyles.ts 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79f9999...78aa7c7. Read the comment docs.

@jurokapsiar
Copy link
Contributor

@sophieH29 let's discuss - we might do something like looking for the focusable in content first and then in the header actions if we did not find anything

@mnajdova yes, the focus indicator should be there - I will leave it up to you if you want to add it in this PR or if you want to create an issue - it probably also depends on if you have the red lines or not.

-changed focus styles for the text buttons
@vercel vercel bot temporarily deployed to staging July 12, 2019 09:31 Inactive
@vercel vercel bot temporarily deployed to staging July 12, 2019 09:32 Inactive
@@ -148,6 +148,12 @@ const buttonStyles: ComponentSlotStylesInput<ButtonProps & ButtonState, ButtonVa
color: textPrimaryColorHover,
},
}),

':focus': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added focus styles for the text button

-improved typings for the shorthand value props in the Dialog
@vercel vercel bot temporarily deployed to staging July 12, 2019 10:08 Inactive
Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

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

one comment

@miroslavstastny
Copy link
Member

We should add screener tests for open dialogs, popups, tooltips... (probably in a separate task)
image

@mnajdova
Copy link
Contributor Author

We should add screener tests for open dialogs, popups, tooltips... (probably in a separate task)

@miroslavstastny I thought about the same thing, so I added steps for the Dialog - the new example and the example with the content... In a separate pr we can add more examples for the popups and tooltips.

@mnajdova mnajdova merged commit 7c194ef into master Jul 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat/dialog-add-icon-slot branch July 12, 2019 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants