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

Adding a dashboard feature (#7) #33

Merged
merged 10 commits into from
Mar 6, 2019

Conversation

lucas-carneiro
Copy link
Contributor

This PR is based on #7, although I couldn't complete it since the API is not available for PRs right now. I did it as a workaround until an account system (and any other features) gets launched.

I made a modal where anyone can create a dashboard, which is a URL with the q parameter set by a autocomplete multiselect component. This component collects all item names through a request to the /api.json route.

However, I didn't have success trying to hit this route. All my tentatives got 404s. I added a mock json file to test the component, so it should work normally in a real scenario.

It's a big PR, so I'm available to answer any questions and fix any bugs before being accepted (if it's good enough for the website, of course).

Cheers

@JessicaYeh
Copy link
Owner

Great work! I looked through the code and it looks pretty solid. I will pull down your branch when I have time later to do a full review and test. (For future reference, screenshots in the pull request are a huge plus. I'll add that to the contribution guidelines)

I have a few questions/comments:

  • Are users able to easily edit a dashboard that they've previously created? Like if they go to their dashboard link, will the dashboard modal load up that URL and let them remove items or add items? I'll probably create a basic mockup later unless you beat me to it, but I'm imagining that the dashboard items should be individual elements on the dashboard modal and each one has an X button that lets the user easily remove it from their dashboard.
  • Looks like the dashboard modal is using a drop down menu that contains the list of all items. That seems kind of unwieldy since there's a ton of items. Perhaps use the Autocomplete component instead?
  • Is the items.json response cached? Or is it reloaded every time the dashboard modal is opened?
  • I believe that URLs have a maximum length. I think it's fairly large so most dashboards probably won't be an issue, but would be nice to do some testing with that (perhaps add a limit to the number of items allowed in a dashboard)

@JessicaYeh JessicaYeh self-requested a review March 4, 2019 23:09
@lucas-carneiro
Copy link
Contributor Author

Thanks for the feedback! Next time I will add screenshots. This one was my first coding PR (I only made readme PRs before), so I wasn't sure how to describe all the work done.

Answers:

  • No, I didn't add any edit features. I could change the modal to check if the URL matches a dashboard URL, and let users edit them. Item removal would be easy to add, since it's already possible to remove any items inside the autocomplete component.
  • The component I used is an autocomplete drop down, so it's possible to both select and write items.
  • Yes, it's being cached. Its request is made only at the first time a user opens the modal, and it will happen again in two scenarios: if a user refreshes the page, and if the first request fails.
  • I tested dashboards between 20 and 30 items, but I didn't check how long a URL can be. I searched it while writing this comment, so I can add something to alert and limit dashboards.

@lucas-carneiro
Copy link
Contributor Author

Considering all 176 items inside mocknames.json, an item has an average of 15 characters. So, a user would exceed the URL limit (2000) if them add more than 133 items into a single dashboard (ignoring host length).

Latest commit shows a warning when user exceeds length limit, and also ignores all items above length limit.

capture

@JessicaYeh
Copy link
Owner

Thanks! The screenshot helps a lot. It looks pretty good visually. Ah I see you're using a different package's Select component which provides all that fancy behavior, cool!

Very nice work for a first coding PR! And sorry about the initial long delay to see your PR, GitHub didn't send me an email for some reason that it was created.

I pulled down your branch to check the appearance on mobile, it looks pretty squished:

Small minor issues/suggestions:

  • The width of the Dashboard URL box should be larger, maybe by about 50%
  • The black button and the copy icon don't match the button color/style of the existing buttons on the site, please make the color and style match
  • There is a console warning:
    image
  • The URL should be encoded, i.e. spaces should turn into %20, etc. The max URL length calculation may be inaccurate if it calculates prior to encoding. And the copyable link should show the encoded version, since that makes it more appropriate for sharing (spaces in links can easily break the link when pasted somewhere)
  • I think a better user experience would be to have the Dashboard URL box initially not be visible when they didn't "Create Dashboard" yet. When they click Create Dashboard, that button can disappear and be replaced by the Dashboard URL box in its place. If the user then makes any change to the dashboard by adding or removing items, the Dashboard URL box will disappear again and be replaced by the Create Dashboard button. Since the dashboard URL isn't being updated on the fly, having the Dashboard URL box always be visible could cause confusion for users, because they may not realize they need to click on Create Dashboard again to have the link be updated.

I would like to see the edit dashboard feature be implemented before shipping everything, since it should be a relatively small change and increases usability a lot. But we can get this PR merged into develop first, and then do a separate PR to have that feature implemented.

Let me know if you have any questions about anything or need any assistance.

@lucas-carneiro
Copy link
Contributor Author

I pulled down your branch to check the appearance on mobile, it looks pretty squished:

I set min-width = 300px, based common minimum smartphone widths. I tested the modal on some mobile resolutions before, but I totally forgot these scenarios.

  • The width of the Dashboard URL box should be larger, maybe by about 50%

Changed its width to be the same as the autocomplete input, which is using 75% of the modal width.

screen shot 2019-03-05 at 8 54 20 pm

What do you think?

  • The black button and the copy icon don't match the button color/style of the existing buttons on the site, please make the color and style match

I'm a bit confused on this one. Are you referring to the "Create Dashboard" button? I'm using the primary color of its mui theme. Should it be the same color as the links for GitHub/Discord/Docs?
I didn't noticed that the icon color was wrong, my bad. I will change to be the same as the button after I fix it.

  • There is a console warning:
    image

Done.

  • The URL should be encoded, i.e. spaces should turn into %20, etc. The max URL length calculation may be inaccurate if it calculates prior to encoding. And the copyable link should show the encoded version, since that makes it more appropriate for sharing (spaces in links can easily break the link when pasted somewhere)

How could I forgot it? Thanks a lot for noticing it! Fixed.

  • I think a better user experience would be to have the Dashboard URL box initially not be visible when they didn't "Create Dashboard" yet. When they click Create Dashboard, that button can disappear and be replaced by the Dashboard URL box in its place. If the user then makes any change to the dashboard by adding or removing items, the Dashboard URL box will disappear again and be replaced by the Create Dashboard button. Since the dashboard URL isn't being updated on the fly, having the Dashboard URL box always be visible could cause confusion for users, because they may not realize they need to click on Create Dashboard again to have the link be updated.

Totally agree with you. Done.

I would like to see the edit dashboard feature be implemented before shipping everything, since it should be a relatively small change and increases usability a lot. But we can get this PR merged into develop first, and then do a separate PR to have that feature implemented.

Sure! Do you want me to create an issue or should I wait you to create it after merging this one?

@JessicaYeh
Copy link
Owner

JessicaYeh commented Mar 6, 2019

Awesome thanks for the quick updates!

I'm a bit confused on this one. Are you referring to the "Create Dashboard" button? I'm using the primary color of its mui theme. Should it be the same color as the links for GitHub/Discord/Docs?

Oh, I may have done some custom css for those Github/Discord/API links. It probably should be set in the MUI theme instead. But yeah, I was referring to the color and button style on those buttons. Feel free to leave the Create Dashboard button as is and I will go in and update the appearance after merging and maybe try to make it easier to have consistent buttons.

Sure! Do you want me to create an issue or should I wait you to create it after merging this one?

I'll make the issue after merging this PR and we can link this PR to that issue

src/dashboard-modal.scss Outdated Show resolved Hide resolved
src/dashboard-modal.tsx Show resolved Hide resolved
@lucas-carneiro
Copy link
Contributor Author

Oh, I may have done some custom css for those Github/Discord/API links. It probably should be set in the MUI theme instead. But yeah, I was referring to the color and button style on those buttons. Feel free to leave the Create Dashboard button as is and I will go in and update the appearance after merging and maybe try to make it easier to have consistent buttons.

Ok! I changed the svg color to match your links. I will await for updates.

@JessicaYeh JessicaYeh merged commit 86f19d2 into JessicaYeh:develop Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants