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 more JS stuff to dom.nim #13483

Merged
merged 7 commits into from
Mar 11, 2020
Merged

Add more JS stuff to dom.nim #13483

merged 7 commits into from
Mar 11, 2020

Conversation

treeform
Copy link
Contributor

@treeform treeform commented Feb 24, 2020

  • LocalStorage
  • activeElement
  • innerText
  • some Style properties.

lib/js/dom.nim Outdated Show resolved Hide resolved
@treeform
Copy link
Contributor Author

treeform commented Mar 5, 2020

Let's hope BSD test will not fail for some intermittent reason again related to io selectors.

@timotheecour
Copy link
Member

Let's hope BSD test will not fail for some intermittent reason again related to io selectors.

you can simply rebase, I fixed the CI build.sh failure in https://github.com/nim-lang/Nim/pull/13564/files

pkg: wrong architecture: FreeBSD:12.0:amd64 instead of FreeBSD:12:amd64

@treeform
Copy link
Contributor Author

treeform commented Mar 7, 2020

Ok rebased and all tests pass.

lib/js/dom.nim Outdated Show resolved Hide resolved
lib/js/dom.nim Show resolved Hide resolved
lib/js/dom.nim Outdated Show resolved Hide resolved
@treeform
Copy link
Contributor Author

I think have made the requested changes. Please take a look again when you can.

lib/js/dom.nim Outdated Show resolved Hide resolved
lib/js/dom.nim Outdated Show resolved Hide resolved
lib/js/dom.nim Outdated Show resolved Hide resolved
@treeform treeform requested a review from dom96 March 11, 2020 20:14
@timotheecour
Copy link
Member

timotheecour commented Mar 11, 2020

  • great, LGTM. subsequent PRs could add doc links for other procs (eg https://developer.mozilla.org/en-US/docs/Web/API/Performance/now)
  • @dom96 I see you often use the "change requested" github feature (in other PRs) but IMO that's overkill, as it often stays forever in that state even long after the contributor has addressed the issue (and contributor has no way to undo that state, only you can). IMO the following is 100% sufficient assuming no abuse:
    • reviewer writes a comment
    • contributor replies, and, using best judgement, marks as resolved if comment is fully addressed
    • unless reviewer disagrees and un-resolves that comment, that's the end of the story

@dom96
Copy link
Contributor

dom96 commented Mar 11, 2020

@timotheecour huh? Where do you see that state reported? I am asking for changes, so I select "Changes Requested", I don't see how that negatively affects the PRs.

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants