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

Add "Open in Codepen" button to Treegrid example #1694

Merged
merged 5 commits into from
Dec 22, 2020
Merged

Conversation

spectranaut
Copy link
Contributor

@spectranaut spectranaut commented Dec 18, 2020

Adding the open in Codepen button! This involved moving some SVGs and moving some code.

Preview

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

Looks good!

Approving now, even though it can't be merged until the cspell ignore regex is fixed.

@nschonni Is the following regex correct?
"url\\(\"data\\:image/svg\\+xml.*\"\\)[,;]"

@nschonni
Copy link
Contributor

@carmacleod makes sense to me, or the punctuation could possibly be dropped entirely. I was just trying to make sure the regex didn't get to greedy, but maybe that's not really an issue for this

@carmacleod
Copy link
Contributor

Thank-you, @nschonni - I created #1696 to add [,;] to the end of the regex. Good point about probably not needing to be that specific because it's pretty unlikely that anything else will ever match, however I decided to check for the end punctuation anyhow. 😄

I have no idea how to test whether merging 1696 will allow this PR to pass the spell-check. (but I think it should work...)

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Looks great ... this would have been a perfect opportunity to improve how we set aria-current. I think it should be on the link inside the list item, not on the entire list item. Rendering aria-current on a long item is awkward for screen readers. I'll raise a separate issue for that though. I want to get this merged.

@mcking65 mcking65 merged commit 7351ed1 into master Dec 22, 2020
@mcking65 mcking65 added enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern labels Dec 22, 2020
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Dec 22, 2020
@mcking65 mcking65 deleted the codepen-treegrid branch December 22, 2020 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants