Skip to content

💄 32 - UI feedback - #76

Merged
anncatton merged 10 commits into
developfrom
32-ui-feedback
Mar 15, 2021
Merged

💄 32 - UI feedback#76
anncatton merged 10 commits into
developfrom
32-ui-feedback

Conversation

@anncatton

@anncatton anncatton commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

Addresses all the issues in #32 , except showing only configured providers, that will be handled in a separate ticket.

Comment thread components/NavBar.tsx Outdated
display: flex;
justify-content: space-between;
height: 50px;
height: ${NAVBAR_HEIGHT}px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why isn't this a property in theme?

FACET_MIN_WIDTH,
FOOTER_HEIGHT,
NAVBAR_HEIGHT,
} from '../../../global/utils/constants';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this an existing pattern or did you introduce this? These feel like values that usually get shared to CSS via the theme object.

Comment thread global/utils/constants.ts Outdated
export const FACET_MAX_WIDTH = 270;
export const FACET_MIN_WIDTH = 250;
export const FOOTER_HEIGHT = 47;
export const NAVBAR_HEIGHT = 50;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Repeating comment about css properties belonging in theme.

@anncatton
anncatton requested a review from joneubank March 15, 2021 20:41

@joneubank joneubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for updates. You can get rid of the consts now.

Comment thread components/theme/dimensions.ts Outdated
export const FACET_MAX_WIDTH = 270;
export const FACET_MIN_WIDTH = 250;
export const FOOTER_HEIGHT = 47;
export const NAVBAR_HEIGHT = 50;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we can get rid of these, ya?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh whoops i thought i did. derp

@anncatton
anncatton requested a review from joneubank March 15, 2021 20:50

@joneubank joneubank left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

j'approve

@anncatton
anncatton merged commit 2b49df0 into develop Mar 15, 2021
@anncatton
anncatton deleted the 32-ui-feedback branch March 15, 2021 20:55
@anncatton anncatton mentioned this pull request Mar 16, 2021
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