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

User page toolbar redesign and code improvements #69

Merged
merged 18 commits into from
May 18, 2020
Merged

Conversation

kylerwsm
Copy link
Contributor

@kylerwsm kylerwsm commented May 10, 2020

Problem

User page toolbar redesign, and code de-coupling and improvements.

Solution

  • Re-designing the toolbar and its components while reusing most logic from before.
  • Following the v2 design, the Download icon button will not be shown in mobile view. Also in mobile view, the Create link button will be minimised into an icon button.
  • Convert huge components become single-directory components.

Improvements:

  • (For components which uses app margins but are not able to make use of the ApplyAppMargins container component) App margin values are now exposed using the useAppMargins hook rather than the fetchAppMargins function from before. Doing this allows the consumer components to better manipulate the app margins.
  • Table rows now follow app margins.
  • Removed some redundant eslint-disable statements.
  • Changed some components from using absolute path to relative path, to get better IDE refactor support.
  • Combined tableTheme.js into theme.js. As a result the link row slider now follows the app theme bluish color.

Before & After Screenshots

BEFORE:
image
Before: Mobile view

AFTER:
image
After: Mobile view

Tests

  • Search bar should search links.
  • Add link button should open the dialog to create a new short link.
  • Add link button should be minimise to an icon button on mobile displays.
  • Download button (only displayed on wider displays) should download all links in shown table.
  • When browser width changes, toolbar should collapse/expand gracefully.

Deploy Notes

New dependencies:

  • @types/lodash : Type file for lodash

@kylerwsm kylerwsm requested review from liangyuanruo and yong-jie and removed request for liangyuanruo and yong-jie May 11, 2020 09:07
@kylerwsm kylerwsm marked this pull request as ready for review May 11, 2020 09:08
@yong-jie
Copy link
Member

Quick query: i see that for imports there are some changes that remove paths like '~/a/b/c' to '../../' etc, is this something recommended by prettier etc? i'm seeing deeply nested components importing a really long string of ../../../../ etc when it could be ~/something etc

@kylerwsm
Copy link
Contributor Author

kylerwsm commented May 12, 2020

Quick query: i see that for imports there are some changes that remove paths like '~/a/b/c' to '../../' etc, is this something recommended by prettier etc? i'm seeing deeply nested components importing a really long string of ../../../../ etc when it could be ~/something etc

The relative paths are not a recommendation by prettier but a preference because of better refactor support from VSCode. I also understand the benefits in using absolute paths. I can change it back to absolute paths if that is more desirable.

@yong-jie
Copy link
Member

i've yet to test refactor for the ~ variant, but ../../ one definitely works on intelliJ, so i'm personally fine with this change

@kylerwsm kylerwsm requested a review from yong-jie May 13, 2020 08:39
yong-jie
yong-jie previously approved these changes May 14, 2020
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

lgtm!

@kylerwsm
Copy link
Contributor Author

Force pushing to rebase to develop branch

@kylerwsm kylerwsm requested a review from yong-jie May 18, 2020 06:06
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -83,7 +83,6 @@ export async function generateOtp(req: Express.Request, res: Express.Response) {
} catch (error) {
logger.error(`OTP generation failed unexpectedly:\t${error}`)
res.serverError(jsonMessage('OTP generation failed unexpectedly.'))
return
Copy link
Member

Choose a reason for hiding this comment

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

This was the only change since my last review.

@kylerwsm kylerwsm merged commit deed7cb into develop May 18, 2020
@kylerwsm kylerwsm deleted the user-table branch May 18, 2020 06:15
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.

3 participants