Add scroll_top() #174

rickycodes · 2018-03-29T05:33:55Z

Just floating this for some early feedback, still need to add docs, but thought I'd open a PR...

example usage (which I am doing for my own website: ricky.codes):

let top = Some(window().page_y_offset()).unwrap_or(document().document_element().unwrap().scroll_top());

and then setting:

document().document_element().unwrap().set_scroll_top(top.parse::<u32>().unwrap());

koute · 2018-04-01T22:49:44Z

src/webapi/element.rs

+    }
+
+    /// scrollTop
+    fn set_scroll_top( &self, value: u32 ) {


Hmm... is there any particular reason why the getter returns i32 and the setter accepts u32?

@koute ah, good catch... I guess I should pick one 🤦‍♂️

what do you think makes the most sense here?

In general we should stick with the types from the specs, so it'd be the best if you'd find the relevant IDL in the specs and use the type specified there. (:

@koute the IDL states "unrestricted double"

so I guess u32?

Unrestricted double should be f64 no?
If you scroll down to TypedArray objects table, you can see what the Web IDL types represent.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray

Just want to do some double checking... Here's some example code of mine:

let top: f64 = Some(window().page_y_offset() as f64) .unwrap_or(document().document_element().unwrap().scroll_top());

I can see now why I left it as i32 to start...

page_y_offset is listed as:

does this make sense? or does page_y_offset need a different type as well?

@rickycodes The double type should also be f64, therefore page_y_offset should return f64.

The only difference between double and unrestricted double is that in the WebIDL they guarantee that double will not be NaN, -Infinity, or Infinity (but the type is the same).

Ok, cool. I just updated that as well. If things look good I can update with documentation/links and this should be good to go :)

Oh, I guess it would also make sense to add page_x_offset and scroll_left as well ;)

Yep. Still needs the docs and the links to the specs. (:

Ok, this should be all good now!

koute · 2018-04-03T18:09:58Z

src/webapi/document.rs

@@ -116,4 +116,17 @@ impl Document {
            js!( @(no_return) @{self}.title = @{title}; );
        }
    }
+
+    /// Returns the Element that is the root element of the document (for example, the <html>


The <html> has to be escaped in backticks or it will mess up the docs.

this should be good now, I've also gone back and added backticks that were missed in some of my previous other work.

also updated doc links and the change to document_element() (:
lmk if you spot anything else!

koute · 2018-04-03T18:11:30Z

src/webapi/document.rs

+    /// element for HTML documents).
+    ///
+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/Document/documentElement)
+    // https://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-87CD092


Please link to this spec instead: https://dom.spec.whatwg.org/#ref-for-dom-document-documentelement

Ditto for other links.

koute · 2018-04-03T18:11:55Z

src/webapi/document.rs

+    ///
+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/Document/documentElement)
+    // https://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-87CD092
+    pub fn document_element( &self ) -> Option< HtmlElement > {


According to specs shouldn't this return Option< Element >?

koute

LGTM, although the links to the specs still need to be fixed. (:

koute · 2018-04-04T20:20:22Z

src/webapi/element.rs

+    /// Gets the the number of pixels that an element's content is scrolled vertically.
+    ///
+    /// [(Javascript docs)](https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTop)
+    // https://drafts.csswg.org/cssom-view/#dom-element-scrolltop


For consistency please link to the IDL in the specs; for this particular method in should be:

https://drafts.csswg.org/cssom-view/#ref-for-dom-element-scrolltop%E2%91%A0

(:

koute · 2018-04-04T20:21:00Z

src/webapi/window.rs

+    /// aligned with the top edge of the window's content area.
+    ///
+    /// [(JavaScript docs)](https://developer.mozilla.org/en-US/docs/Web/API/Window/pageYOffset)
+    // https://drafts.csswg.org/cssom-view/#dom-window-pageyoffset


These should also link to the IDL, e.g. in this case https://drafts.csswg.org/cssom-view/#ref-for-dom-window-pageyoffset

rickycodes · 2018-04-04T21:21:42Z

@koute ack! 🤦‍♂️ ok, I hope this covers em'

thanks for catching these!

koute · 2018-04-05T19:58:13Z

src/webapi/window.rs

@@ -190,4 +190,27 @@ impl Window {
            return @{self}.outerHeight;
        ).try_into().unwrap()
    }
+
+    /// he read-only Window property pageYOffset is an alias for scrollY; as such, it returns


he -> The (:

koute · 2018-04-05T19:58:28Z

Besides the typo LGTM. (:

Add scroll_top()

6d2ebbe

koute reviewed Apr 1, 2018

View reviewed changes

rickycodes added 4 commits April 1, 2018 22:28

Make values f64

bf3b979

Make this f64 also

ca36052

Add scrollLeft and pageXOffset

7f8834f

Add documentation

e373408

koute reviewed Apr 3, 2018

View reviewed changes

Fix

2c99cfa

koute reviewed Apr 4, 2018

View reviewed changes

Update IDL links

02b9fac

rickycodes force-pushed the add-scroll-top branch from 3e496df to 02b9fac Compare April 4, 2018 21:19

koute reviewed Apr 5, 2018

View reviewed changes

koute merged commit 003ef26 into koute:master Apr 5, 2018

rickycodes deleted the add-scroll-top branch October 7, 2019 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scroll_top() #174

Add scroll_top() #174

rickycodes commented Mar 29, 2018

koute Apr 1, 2018

rickycodes Apr 1, 2018

koute Apr 1, 2018

rickycodes Apr 1, 2018 •

edited

Loading

teovoinea Apr 1, 2018

rickycodes Apr 2, 2018 •

edited

Loading

Pauan Apr 2, 2018

rickycodes Apr 2, 2018 •

edited

Loading

koute Apr 2, 2018

rickycodes Apr 3, 2018 •

edited

Loading

koute Apr 3, 2018

rickycodes Apr 3, 2018

rickycodes Apr 3, 2018

koute Apr 3, 2018

koute Apr 3, 2018

koute left a comment

koute Apr 4, 2018

koute Apr 4, 2018

rickycodes commented Apr 4, 2018

koute Apr 5, 2018

koute commented Apr 5, 2018

Add scroll_top() #174

Add scroll_top() #174

Conversation

rickycodes commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickycodes Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickycodes Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickycodes Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickycodes Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koute left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickycodes commented Apr 4, 2018

Choose a reason for hiding this comment

koute commented Apr 5, 2018

rickycodes Apr 1, 2018 •

edited

Loading

rickycodes Apr 2, 2018 •

edited

Loading

rickycodes Apr 2, 2018 •

edited

Loading

rickycodes Apr 3, 2018 •

edited

Loading