Skip to content

Add default options argument to sidebar documentation #19

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

Merged
merged 13 commits into from
Sep 23, 2020

Conversation

dev10110
Copy link
Member

Proposed changes

It took me an (embarrassingly long) while to realise that the lack of var options= {} was the reason my sidebar wasn't working. Hopefully, this documentation update can help someone going forward.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dev10110
Copy link
Member Author

@Smankusors hopefully this is the right place to make this change?

Copy link
Member

@Smankusors Smankusors left a comment

Choose a reason for hiding this comment

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

looks fine for me.

well... actually... I just looked the documentation.... And it's not just sidenav... collapsible, dropdown... uh oh, literally every JS component have this doc problem 😰

@dev10110
Copy link
Member Author

gosh indeed - I think I've corrected it where I could find it (i searched for elem, options) not sure if there are others.

@DanielRuf
Copy link

See also #1

@DanielRuf DanielRuf requested a review from doughballs September 1, 2020 21:07
Copy link

@doughballs doughballs left a comment

Choose a reason for hiding this comment

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

Think you missed collapsible, see my list at #1

This fix is fine, it does the main job of removing the error. This is fix #2 from my suggestions, whereas I was edging towards fix #1 - removing options rather than declaring it - as I do think that declaring raises other questions. For instance, what we will essentially see as a new user is this:

document.addEventListener('DOMContentLoaded', function() {
    var elems = document.querySelectorAll('select');
    var options = {};
    var instances = M.FormSelect.init(elems, options);
  });

  // Or with jQuery

  $(document).ready(function(){
    $('select').formSelect();
  });

That is - the jQuery init has no reference to options.

Do we need to declare options again in the jQuery init? Should we bother declaring options at all? It is unnecessary, seeing as we can simply set our options with an object var instances = M.FormSelect.init(elems,{});

Further notes - the docs confusingly use instances as well as instance - this can easily trip people up. There's is no need to declare instance when we can just use instances[0], and also, a much cleaner init when you don't need to get the instance is to not even declare it at all:

M.FormSelect.init(document.querySelectorAll('select'))

This single line replaces three lines where there are not options to set:

var elems = document.querySelectorAll('select');
var options = {};
var instances = M.FormSelect.init(elems, options);

And a further thought, is how useful it is to keep redeclaring the same variable over and over, instances and elems...

@dev10110
Copy link
Member Author

dev10110 commented Sep 2, 2020

Hmm I see what you're trying to say.

Would it be worth mentioning these caveats in a single reference page somewhere, but still ensuring the documentation runs without errors on copy-paste?

I don't mind most of your options, I just dislike

var instances = M.FormSelect.init(elems,{})

Since as a user I wouldn't have a clue what the second argument was or why it was there.

@dev10110
Copy link
Member Author

dev10110 commented Sep 2, 2020

Also I haven't looked too hard, but how should options be specified for jQuery?

@doughballs
Copy link

doughballs commented Sep 2, 2020

Also I haven't looked too hard, but how should options be specified for jQuery?

Like this:

$('select').FormSelect(options)

Shown here with the Modal component, option declaration taken outside of the document.ready so that both inits can use them (comment out whichever you prefer.

@doughballs
Copy link

Hmm I see what you're trying to say.

Would it be worth mentioning these caveats in a single reference page somewhere, but still ensuring the documentation runs without errors on copy-paste?

I don't mind most of your options, I just dislike

var instances = M.FormSelect.init(elems,{})

Since as a user I wouldn't have a clue what the second argument was or why it was there.

Strong yes to a standalone page explaining the inits, how to use use them, what options are, how they differ from methods etc. Basic theory such as:

Each component has a set of default options that it is initialised with. For instance, initialising modal without options like this...

$('.modal').modal();

...is the same as writing:

$('.modal').modal({
  opacity: 0.5,
  inDuration: 250,
  outDuration: 250,
  onOpenStart: null,
  onOpenEnd: null,
  onCloseStart: null,
  onCloseEnd: null,
  preventScrolling: true,
  dismissible: true,
  startingTop: '4%',
  endingTop: '10%'
});

..where options is an object {} of key:value pairs. Please note that values can be strings, booleans, numbers & functions -refer to each component page for a full list of options.

In addition, it would be good to talk about multiple components on the same page - 2 modals with different options. How to approach this. It's not immediately obvious to beginner that $('.modal').modal() in fact returns an array of all elements with a matching classname. For a single modal, user could use something like:

var modal = $('.modal').modal(options);
console.log(modal[0])

To get the instance, and if there were two on the page, modal[1] would refer to the second, and so on. And then we get into the territory of perhaps giving components IDs, and initialising separately.

Sorry, I waffle but I use Materialize so much and run into these things all the time, fo me it is easy to use but I remember at the start that the docs were not helpful.

Bottom line - your solution works, but we have left the jQuery explanation a little short (and inconsistent).

@DanielRuf
Copy link

Bottom line - your solution works, but we have left the jQuery explanation a little short (and inconsistent).

How can we solve this in the best way? Any ideas / proposed solutions?

@dev10110
Copy link
Member Author

dev10110 commented Sep 6, 2020

As you can probably tell, I'm rather new to materialise (and web dev generally) and thus am probably not the best person to write up all these details.

I can transcribe most of what has been mentioned in the thread (into a new page called 'working with JS objects' or similar, or editing 'auto init') if it could help more knowledgeable people also add details.

Not sure how that works with a PR though - would you need to be collaborators on my GitHub fork? Or can you also submit updates to the same PR?

@Smankusors
Copy link
Member

Smankusors commented Sep 6, 2020

I think we can do something like this instead (adding comment "Options here"), for example in the collapsible documentation

document.addEventListener('DOMContentLoaded', function() {
  var elems = document.querySelectorAll('.collapsible');
  var instances = M.Collapsible.init(elems, {
    // Specify options here
  });
});

// Or with jQuery

$(document).ready(function(){
  $('.collapsible').collapsible({
    // Specify options here
  });
});

with this, the new user will know where to put the options...

Now.... if they don't know how to write options argument............ then..... 🙊

@doughballs
Copy link

doughballs commented Sep 6, 2020

I think we can do something like this instead (adding comment "Options here"), for example in the collapsible documentation

document.addEventListener('DOMContentLoaded', function() {
  var elems = document.querySelectorAll('.collapsible');
  var instances = M.Collapsible.init(elems, {
    // Specify options here
  });
});

// Or with jQuery

$(document).ready(function(){
  $('.collapsible').collapsible({
    // Specify options here
  });
});

with this, the new user will know where to put the options...

Now.... if they don't know how to write options argument............ then..... 🙊

This sounds good - it answers both the options error and jQuery question.

Perhaps underneath the jQuery script - on every component page - we have a link:

'For more details on using options, see https://materializecss.com/options'

And this would link to a new page that we create, with a broader explanation of options and how they are used (which links back to @dev10110's earlier post)

@dev10110 what do you think about this over declaring options method?

@DanielRuf
Copy link

Maybe we can use a partial which is included so we have to write the text only once:
https://jade-lang.com/reference/includes

Let me know if you need any help with this.

Regarding push / write access for others, there is a checkbox in the PR settings to allow that others can push to your fork branch.

I do not see if this is checked / activated by you.
And I'll have to check if the permissions for all others is correct (write permissions).

@doughballs
Copy link

Maybe we can use a partial which is included so we have to write the text only once:
https://jade-lang.com/reference/includes

Let me know if you need any help with this.

Regarding push / write access for others, there is a checkbox in the PR settings to allow that others can push to your fork branch.

I do not see if this is checked / activated by you.
And I'll have to check if the permissions for all others is correct (write permissions).

Yes - good thinking. DNR!

Obviously we will need to create the page first, and the content, which may take a bit of back and forth, so for this PR we could just fix the issue and treat the new page as a separate concern.

@dev10110
Copy link
Member Author

dev10110 commented Sep 6, 2020

Alright sounds good.

Ill make an update to the jQuery part of the docs in this PR, and open a new one for the extra text on options.

I did see and tick the checkbox and so that's awesome.

Thanks guys!

@dev10110
Copy link
Member Author

dev10110 commented Sep 6, 2020

@DanielRuf seems like ill need a hand getting the page working - as far as I can tell, the documentation right now is compiled using jade, but the main content is included as a html file. I don't see how to produce and reuse a block of text within the html files.
I could add the notes to the bottom of the page (before the footer) but that seems like an odd place to put it.

@dev10110
Copy link
Member Author

dev10110 commented Sep 6, 2020

also, jquery updates done!

@DanielRuf DanielRuf added the documentation Improvements or additions to documentation label Sep 9, 2020
@DanielRuf
Copy link

What is the current status of this PR?

@dev10110
Copy link
Member Author

From my POV: this is ready to go. For the extra documentation on how init works, I tried it, but couldnt figure out how to use the same snippet in multiple places using Jade. #19 (comment)

@DanielRuf
Copy link

@Smankusors what do you think? Can it be merged?

@Smankusors
Copy link
Member

@Smankusors what do you think? Can it be merged?

hmm the chips documentation, honestly it could be merged now and fix it later I suppose... 🤔

that example can be copy from jquery to non jquery, but IMO it looks too long compared to other component documentations. perhaps in the future we should have tab to select non jquery version and jquery version though. 💭

for the multiple $(document).ready on the chips documentation, I think we can merge it as one instead. I could do it if it's okay for you @DanielRuf @dev10110

@dev10110
Copy link
Member Author

Ah yes indeed. That would be much appreciated @Smankusors

@DanielRuf
Copy link

Feel free to do so @Smankusors.

Copy link
Member

@Smankusors Smankusors left a comment

Choose a reason for hiding this comment

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

alright I think it's done 🙂

@DanielRuf DanielRuf merged commit 898cd1a into materializecss:v1-dev Sep 23, 2020
@DanielRuf
Copy link

Congrats on your first merged PR @dev10110.

I've sent you an invitation to this org and the members group.
You can see and accept it at https://github.com/materializecss.

@dev10110
Copy link
Member Author

dev10110 commented Sep 23, 2020

Woohoo thanks Daniel, Anthony!

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

Successfully merging this pull request may close these issues.

4 participants