Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Fixed issue with File Drop #1073

Merged
merged 5 commits into from
Sep 27, 2017
Merged

Fixed issue with File Drop #1073

merged 5 commits into from
Sep 27, 2017

Conversation

Blackbaud-SteveBrush
Copy link
Member

Depending on the compileMode, the Avatar Component (read: File Drop) looks different. This is due to how the HTML is rendered. During jit (or skyux serve), the HTML template is parsed differently, and the innerHTML of the file drop button is extracted and added outside of it. During an aot build, the innerHTML of the button is preserved. I'm assuming this is because the webpack loaders used for each scenario are different.

(Note that placing block elements inside a button is considered invalid.)

If compile mode is jit or you type skyux serve:

screen shot 2017-09-15 at 1 21 14 pm

If compile mode is aot:

screen shot 2017-09-15 at 1 21 28 pm

(This is why the live documentation looks fine, but serving the docs locally with skyux serve looks different.)

@codecov-io
Copy link

codecov-io commented Sep 15, 2017

Codecov Report

Merging #1073 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1073   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         355     355           
  Lines        6577    6577           
  Branches      842     842           
======================================
  Hits         6577    6577

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 81906aa...2e1368b. Read the comment docs.

@Blackbaud-BobbyEarl
Copy link

Is there any accessibility concern by changing this from a button? Perhaps @Blackbaud-MattGregg has some insight?

@Blackbaud-MattGregg
Copy link
Contributor

Yes, I think this is going to be problematic looking at the changes (I think I'm looking at the right thing) b/c there isn't anything that is going to be keyboard focusable without the button OR communicate this function exists (i.e., the

) Additionally, I've found this one problematic b/c there isn't any communication of state (i.e., has/doesn't have an image). @Blackbaud-SteveBrush want to look at this together?

@Blackbaud-SteveBrush
Copy link
Member Author

@Blackbaud-BobbyEarl @Blackbaud-MattGregg I was worried that would be an accessibility issue. Let me look at it further; I'll reach out to you, Matt, if I encounter any issues. Thanks!

@Blackbaud-SteveBrush
Copy link
Member Author

@Blackbaud-BobbyEarl @Blackbaud-MattGregg This is ready for another look!

@Blackbaud-MattGregg
Copy link
Contributor

Looks better. will be reachable via keyboard and has text for it's function. Back to where it was before. We could look at other A11y improvements later.

@bobbyearl
Copy link

@Blackbaud-SteveBrush Should the cursor still be a pointer as is with the current behavior?

@Blackbaud-SteveBrush
Copy link
Member Author

Good catch, @Blackbaud-BobbyEarl. Fixed!

@Blackbaud-SteveBrush Blackbaud-SteveBrush merged commit ec7ab5d into master Sep 27, 2017
@Blackbaud-SteveBrush Blackbaud-SteveBrush deleted the fix-avatar branch September 27, 2017 18:47
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.

5 participants