Can we move the disable logic to the progress group component? Ideally we would only pass in activeIndex and number of buttons. And just disable all buttons with index larger than activeIndex

I was thinking about this too, but activeIndex could be less than the most activate-able index. For example, you could get to step 3, then go to step 2, but you want step 3 to still be clickable. Wanted to flesh it out, but lunch break was coming to an end.

What do you think about:

ProgressGroup.propTypes = {
  activeIndex: PropTypes.number, // "clicked"
  lastValidIndex: PropTypes.number, // all values after this will be "disabled"
  steps: PropTypes.arrayOf(PropTypes.shape({
    text: PropTypes.string.isRequired,
    onClick: PropTypes.func,
  })),
};

Also, do you think text should even be an option, or should the component just generate it?

ProgressGroup.propTypes = {
  count: PropTypes.number.isRequired, // number of elements to create
  activeIndex: PropTypes.number, // "clicked"
  lastValidIndex: PropTypes.number, // all values after this will be "disabled"
  onClicks: PropTypes.arrayOf(PropTypes.func), // assigned to buttons based on index
};

I don’t think steps is necessary - you can automatically generate n number of buttons labeled from 1 to n, given that you pass in n in the props. And we’ll need a callback function that is called whenever the page is changed so that whoever is listening to that callback can know what page it’s in.

And whether we can click a page that’s after the current page is something we should confirm with design

I'll go with my second idea then, and just allow one callback that all the button clicks call which provides the current page index.

^^^ Can we only have one onClick function?

pseudo code of how we could have just one onClick function:

//Progress Group
const onClickFunction = (page) => console.log("Page: ", page);
<ProgressGroup>
  <ProgressButton page=1 onClick={onClickFunction} /> //page 1
  <ProgressButton page=2 onClick={onClickFunction} /> //page 2
<ProgressGroup />

// ProgressButton component
const ProgressButton = ({ page, onClick }) => (
  const callback = () => onClick(page);
  <button onClick={callback}>BUTTON TEXT</button>
)

I think it would require an additional component.

In the end, this method would still require a separate callback function per button (callback), but the difference is that it would be abstracted inside the ProgressGroup component - whatever is using the ProgressGroup component would not have to know that implementation detail

Why are we 0-indexing here? Wouldn't it make more sense to start from one since page numbers also start with 1? I think it would be easier that way

1-indexing is a sin!

tbh though 0-indexing is better here - while it seems a bit confusing, the text in the button is purely cosmetic, and when you are writing up the logic with no regards to the displayed numbers it makes more sense to start at 0 to match array loops, etc.

I still don't really understand why 0-indexing is better but can we at least change the API so that the user doesn't have to know anything about "index"? So changing activeIndex to activePage and lastValidIndex to lastValidPage? Don't really see why the user has to know anything about how the buttons are stored.

The only users are us, not the users of the site, and as far as writing code goes 0-indexing is correct and anything else is honestly confusing - all lists start at 0, why should this list start at 1?

http://www.cs.utexas.edu/users/EWD/transcriptions/EWD08xx/EWD831.html

Users don’t have to know how it’s stored but we do

The users are not only us - the users include devs who do not know any implementation detail of this component (e.g. new devs next year who may want to reuse our components, or those who weren’t involved in developing this component but need to use it to build another component). When writing base components it’s really important to think about reusability and good API to ultimately save time in the future.

By forcing users to specify an index, we are revealing a detail of our implementation which is completely unnecessary. All they want to do is generate progress buttons from page 1 to n and specify the current page and last active page.

And about 0-indexing i don’t really care but if we do go forward with changing the API to specify page instead of index, you’d literally have to do i+1 whenever you reference i. So starting with 0 seems especially unnecessary compared to just starting with 1. Moreover we are not really “storing” the buttons in a list, we’re just rendering a list of buttons using a for loop. If the for loop started with 1, the first button would still be 0-indexed - buttons[0] - so wouldn’t be doing anything crazy here. Same old 0-indexed list, but generated with a for loop that starts at 1 instead of 0.

When writing base components it’s really important to think about reusability and good API to ultimately save time in the future.

0-index will do exactly that, it is the standard all APIs are based on.

By forcing users to specify an index, we are revealing a detail of our implementation which is completely unnecessary. All they want to do is generate progress buttons from page 1 to n and specify the current page and last active page.

Thats a really weird way of looking at it - I want to generate a set of n elements, and iterate through them.

And about 0-indexing i don’t really care but if we do go forward with changing the API to specify page instead of index, you’d literally have to do i+1 whenever you reference i.

What? Why would you specify page by the name? Just reference the order. API is simple enough: https://github.com/nwhacks/nwhacks2019/pull/21/files#diff-35b1e54975e81f41e3665bf4708f4329R15

Moreover we are not really “storing” the buttons in a list, we’re just rendering a list of buttons using a for loop.

This is a list! A list of pages. If I want the first of an list, I want the 0th of that list. I imagine most developers would use that standard. I've literally never seen any API of any sort start with a 1.

but generated with a for loop that starts at 1 instead of 0.

You should read the article I linked - if we start at 1, why not 10, or 50? 0 is the 0 value, it is the beginning 🥇

Also things like:
0 - 1 = -1, obviously invalid
1 - 1 = 0, .... is that invalid? but all code uses the 0-index, why is this invalid?

This is all really philosophical. In summary: 0-index is the standard, it makes sense, users expect it in APIs (I never expect a 1-indexed API)... there's literally no reason to index these by their name.

the users include devs who do not know any implementation detail of this component

these devs will expect a 0-index, because that's just how things work lol. It's the 1-index that will stump anyone who has experience with working with code and APIs.

This API is definitely an improvement, but I'm still not digging https://github.com/nwhacks/nwhacks2019/pull/21/files#diff-35b1e54975e81f41e3665bf4708f4329R15 - where you have to get the page number by reading the text value of the element (parseInt(e.currentTarget.textContent, 10) - 1). We want to get the page number/index as the argument, nothing more.

We could easily account for that by declaring another callback function like so:
const func = () => onClick(i)
and passing in this function as the onClick function:
onClick={func}

We want to get the page number/index as the argument, nothing more.

ah, was lazy. Good idea

886cfcd