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

Progress button group #21

Merged
merged 5 commits into from Jul 19, 2018

Conversation

@bobheadxi
Copy link
Contributor

bobheadxi commented Jul 11, 2018

Lunch break 🤷‍♂️

👷 Changes

progressgroup

💭 Notes

Not sure what best to do for inputs to generate the component

Current usage:

      <ProgressGroup
        activeIndex={1}
        steps={[
          { text: '1', onClick: ButtonCallback },
          { text: '2', onClick: ButtonCallback },
          { text: '3', onClick: ButtonCallback, disabled: true },
          { text: '4', onClick: ButtonCallback, disabled: true },
        ]} />

PR currently pointed at #20 for a more accurate diff.

UPDATE: see #21 (comment)

🔦 Testing Instructions

make web

# go to http://localhost:8080/ui_demo
@bobheadxi bobheadxi requested review from chamkank, mingyokim and sherryyx Jul 11, 2018
@bobheadxi bobheadxi changed the base branch from master to web/primary-button Jul 11, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 11, 2018

Coverage Status

Coverage decreased (-1.5%) to 29.487% when pulling 886cfcd on web/progress-button-group into ba8666a on master.

steps={[
{ text: '1', onClick: ButtonCallback },
{ text: '2', onClick: ButtonCallback },
{ text: '3', onClick: ButtonCallback, disabled: true },

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 11, 2018

Contributor

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

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 11, 2018

Author Contributor

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,
  })),
};

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 11, 2018

Author Contributor

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
};

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 11, 2018

Contributor

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

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 11, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 12, 2018

Contributor

^^^ 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

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 17, 2018

New API:

class FrontEndComponents extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      active: 3,
    };
  }

  switchProgress = e => this.setState({ active: parseInt(e.currentTarget.textContent, 10) - 1 })

  render() {
    const { active } = this.state;
    return (
      <div>
          <ProgressGroup
            count={10}
            onClick={this.switchProgress}
            activeIndex={active}
            lastValidIndex={7}
          />
       </div>
    )
  }
}

progressgroup-2

@bobheadxi bobheadxi requested a review from mingyokim Jul 17, 2018
@bobheadxi bobheadxi changed the base branch from web/primary-button to master Jul 17, 2018
@bobheadxi bobheadxi dismissed mingyokim’s stale review Jul 18, 2018

stale whale

Copy link
Contributor

mingyokim left a comment

almost there!!


const ProgressGroup = ({ count, onClick, activeIndex, lastValidIndex }) => {
const buttons = [];
for (let i = 0; i < count; i += 1) {

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 18, 2018

Contributor

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

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor

1-indexing is a sin!

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 19, 2018

Contributor

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.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 19, 2018

Contributor

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.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor

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.

buttons.push(
<button
key={i}
onClick={onClick}

This comment has been minimized.

Copy link
@mingyokim

mingyokim Jul 18, 2018

Contributor

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}

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor

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

ah, was lazy. Good idea

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 19, 2018

Author Contributor
@bobheadxi bobheadxi dismissed mingyokim’s stale review Jul 19, 2018

We are not designing for a publicly usable library - it's really a waste of time arguing over things that do not improve functionality. The variable is called index - it means index- I would be really concerned about bringing on a developer who can't wrap his or her mind around that concept that 0-index means the first page, and so on. I'm going to merge this now - change it yourself if you absolutely must, but since no other issues have been raised I think we can move on 💸

@bobheadxi bobheadxi merged commit 1fc3999 into master Jul 19, 2018
5 checks passed
5 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-1.5%) to 29.487%
Details
@bobheadxi bobheadxi deleted the web/progress-button-group branch Jul 19, 2018
@mingyokim

This comment has been minimized.

Copy link
Contributor

mingyokim commented Jul 19, 2018

@bobheadxi whoops sorry I should've approved (i was on my phone) - i'm okay with either approach since the functionality is the same as you've mentioned.

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 19, 2018

All good, thanks for the comments!

@mingyokim

This comment has been minimized.

Copy link
Contributor

mingyokim commented Jul 19, 2018

@bobheadxi wait i forgot to do a sanity check before you merged it, and it doesn't seem like lastActiveIndex is working - on ui_demo page, I can still click the page 9 and 10? Can you confirm this bug?

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 19, 2018

Ah, I forgot to change disabled from a class to a button trait after merging from the other PR. I'll put up a patch soon (and write some tests 🤕 )

@mingyokim

This comment has been minimized.

Copy link
Contributor

mingyokim commented Jul 19, 2018

Okay thanks! And I agree, would definitely love to see some tests on this component (y)

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 19, 2018

Tracking in #25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.