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

Dashboards + misc browser UI adjustments #8

Merged
merged 23 commits into from Nov 1, 2018
Merged

Dashboards + misc browser UI adjustments #8

merged 23 commits into from Nov 1, 2018

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Oct 10, 2018

TODO

Dashboards

  • smarter url parsing / signing algo (relative and absolute path handling)
  • better error handling / display
  • sign links in md files
  • Docs in UserDocs.md
  • validate vega specs

Misc browser UI adjustments

  • summary stats: proper styling
  • show image thumbnails
  • preview window: close button
  • fix list overflow at narrow widths

@nl0 nl0 requested a review from akarve October 10, 2018 18:53
@akarve akarve requested a review from meffij October 10, 2018 19:54
*
* @param {Object} md Remarkable instance
* @param {Object} options
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks slightly incorrect to me -- I think as it is, this declares a function of 3 parameters, when really you want a function with 1 parameter that has 2 destructure parameters. See "Documenting a destructuring parameter" in http://usejsdoc.org/tags-param.html

Also, if you could document the return type, I would appreciate that.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -63,6 +80,8 @@ const adjustLinks = (md) => {
md.renderer.rules.link_open = (tokens, idx) => {
const t = tokens[idx];
const title = t.title ? ` title="${escape(t.title)}"` : '';
// TODO: sign
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think we need to sign the links as well, not only images, so i've left this TODO

Copy link
Contributor

Choose a reason for hiding this comment

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

OK so this PR isn't done till this is fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct



const DEFAULT_URL_EXPIRATION = 5 * 60; // in seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't this imported from /constants?

Copy link
Member Author

Choose a reason for hiding this comment

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

because it's a default value. if we want to configure this, we should add a config value and then use it when instantiating the provider in app.js

@nl0 nl0 changed the title WIP Dashboards WIP Dashboards + misc browser UI adjustments Oct 11, 2018
@nl0
Copy link
Member Author

nl0 commented Oct 30, 2018

@akarve now this one looks complete

@nl0
Copy link
Member Author

nl0 commented Oct 31, 2018

docs added

@nl0 nl0 changed the title WIP Dashboards + misc browser UI adjustments Dashboards + misc browser UI adjustments Oct 31, 2018
@akarve akarve merged commit 0ea7fcd into master Nov 1, 2018
@akarve akarve deleted the dashboards branch November 1, 2018 20:56
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

3 participants