search

AdaephonBen / cryptex

2 stars 0 forks source link

Leaderboard Front-End #21

Closed RachitKeertiDas closed 4 years ago

RachitKeertiDas commented 4 years ago

Add Leaderboard Front-end Interface Leaderboard Interface is responsive. npm run lint ran successfully. Add Basic Team Interface( usable, not fully complete)

RachitKeertiDas commented 4 years ago

Added the requested changes. Pls,review, and squash and merge.

StrangeAmoeba commented 4 years ago

If possible, I'd like the branch to be renamed to something more appropriate @RachitKeertiDas .

RachitKeertiDas commented 4 years ago

Last time. You know what happened :)

AdaephonBen commented 4 years ago
AdaephonBen commented 4 years ago

Also this isn't an enhancement, it's more of a feature. Use the correct label.

RachitKeertiDas commented 4 years ago

Also this isn't an enhancement, it's more of a feature. Use the correct label.

@AdaephonBen , Github's enhancement label stands for "New feature/request"

RachitKeertiDas commented 4 years ago
  • Weird spacing after "Leaderboard" title. Weird hack to solve that (because a proper method doesn't exist): give the title a margin-right: -xpx where x is the letter spacing you applied on the title.
  • Title text wraps around on some mobile devices. Fix that.
  • The lack of spacing between the search text box and the search icon is unnerving. Fix that.
  • Add the username display for teams. Make rows hoverable and clickable and display usernames on clicking.
  • Don't use sections. Use Flexes and Boxes everywhere.
  • Get a plugin that truncates end of line spaces and use it.
  • Prettify your code with prettier before PR-ing

Will fix certain issues.
However:

  1. Intended behaviour. 4.Still a point of discussion.
npalladium commented 4 years ago
  1. Intended behaviour.

If that search bar was intended behavior, please explain the design decisions behind it with examples from other projects. I have not encountered such a search bar in any of the products I have viewed favorably over the years.

Usually, the icon is a seamless extension of the search bar or is an extension with a different background. Otherwise, it is separated. Ref images below.

image image image image

4.Still a point of discussion.

Also, I believe the hover-over/click-on display of extra information has been approved and no longer a point of discussion. As I'm not heavily involved in the front-end part of this project, please let me know if I'm wrong. I would also be interested in the disadvantages of doing so to the players.

Github's enhancement label stands for "New feature/request"

You are technically right that the enhancement label is used for new features. However, it is rarely, if at all, used for the core features. Note that we are just working on the various must have parts of the portal and not on any new features. Pedantry, which is what is going on over here, would therefore dictate we not add a label claiming enhancement as it is not enhancement but an initial stage in creation. Also, in a lot of the projects, it is reserved for external contributors as you just check the labels to see the PRs from external contributors.

Drop labels from now on until we open source the project or believe that labels are actually of value. I'm up for discussing why the label is of value but not over here.

CrazyByDefault commented 4 years ago

Agreed with most of the expected behaviour described by @npalladium above, although I wasn't involved in the discussions regarding the frontend.

Also:

Squash your commits, please. There are 5 commits that solely exist because you pulled from origin.

Rename your branch.

Re code quality and linting: would appreciate CI (GitHub actions or external) to test linting on any MR.

npalladium commented 4 years ago

Squash your commits, please. There are 5 commits that solely exist because you pulled from origin.

Agreed. We are squashing commits while merging the PR now. Not ideal and would prefer already squashed commits.

Re code quality and linting: would appreciate CI (GitHub actions or external) to test linting on any MR.

We have githooks for now. But a CI would be nice. Opening an issue for this. @CrazyByDefault, help us out in this front.

RachitKeertiDas commented 4 years ago

Thank you for the loads of advice. I also see a lot of duplicate commits.
I' m closing the PR for now.
Will open a new one with fixed commits.