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

Initial discover implementation #8

Merged
merged 33 commits into from
Feb 20, 2019
Merged

Initial discover implementation #8

merged 33 commits into from
Feb 20, 2019

Conversation

rmisio
Copy link
Contributor

@rmisio rmisio commented Feb 12, 2019

This is an initial pass-through of the Discover experience. It's really just 8 category boxes.

Notes:

  • Right now it's really slow. This is due to the search endpoint being absurdly slow. Tyler is looking into it.

@rmisio rmisio changed the title Initial discover implementation WIP - Initial discover implementation Feb 12, 2019
}

processProps(props = {}) {
import(`../../${props.path}`).then(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of slight of hand here... because it appears that we're dynamically importing the modal each time, but since the modal open action is forcing you to pass in a Component, it means you need to manage importing it and then when the root imports it, it will come from cache. The reason I did that is so we're not forcing every modal to be dynamically loaded. We're leaving it up to the parent, which can lazy load it in, or can have it be part of a larger chunk.

@@ -0,0 +1,21 @@
import React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an example of a modal implementation. There's nothing really special about a Modal component. It's just a regular component that you could render in a modal window. The only difference is it needs a modulePath property so the ModalRoot knows where to get it.

With that said... I don't think modals need to live in the modals folder. They're just regular components that should live where is most appropriate for them in the folder structure.

This one happens to live in the modals folder, but that's just because it's a special very specific modal. In other words, the modals folder here is more of "things having to do with modals", rather than how we do it in v2 which is "all modals go here".

@@ -0,0 +1,47 @@
import React, { Component } from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A simple modal test page that can be accessed via the url /modals. It's basically in place cause I'll create a task for someone to style modals in the v2 style (all that scrolling magic, etc...) + responsively style them.


// selectors

export const getCategories = createSelector(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redux selectors - read about them! Basically often times the shape of the state tree for the reducer is slightly different than the ideal one for the UI. The selector is a way to go from the former to the latter. And, it utilizes memoization, so it does it in an efficient way.

heading = <h1 className="h2 CategoryBox-heading">{props.heading}</h1>;
}

if (['tablet', 'desktop'].includes(props.breakpoint)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at how props.breakpoint is used in this file to see an example of how we use conditional markup depending on the breakpoint.

// idea is to eliminate page jumps as content loads in.
height: 635px;

@include mq(740px, 770px) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the sass-mq lib for inline media queries.

);
}

Spinner.defaultProps = {
Copy link
Contributor Author

@rmisio rmisio Feb 12, 2019

Choose a reason for hiding this comment

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

We should probably use defaultProps and propTypes on most components at least as an implicit way of documenting things.

@rmisio rmisio changed the title WIP - Initial discover implementation Initial discover implementation Feb 12, 2019
@import '../../styles/variables.scss';
@import '../../styles/mixins.scss';

.ListingsGrid {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this guy pretty fluid where it will just size up to the container you put it in. We could build up the responsive styling so it looks different on smaller devices. I imagine, we'd probably just want to stack the cards. I think that's what the Haven designs have.

@import './variables.scss';
@import './mixins.scss';

// The idea of this class is so that rather than hard-coding some parent element to have a set width,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to be really modular here with page padding and widths so we don't get burned like in v2 where we assumed a certain page width and by the time I went to try and make a full screen grid, I found major remodeling / refactoring was necessary. I think the navbar, chat, modal, etc... all would'v required some re-work. So, let's keep that in mind from the start. Hopefully, these classes are a step in the right direction.

@@ -0,0 +1,109 @@
import { get } from 'axios';
import { SEARCH_RANDOM_URL } from 'util/constants';
Copy link
Contributor Author

@rmisio rmisio Feb 15, 2019

Choose a reason for hiding this comment

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

@jjeffryes One thing you might notice is that the project is set-up to support absolute imports relative to the src folder, which I think is a lot cooler than the standard where imports are all relative to a file.

With the formr, if you move the file, you don't need to change all the imports.

The one exception I've been doing is if it's a file in the same folder, then I may do import blah from './blah.js. It really depends whether you think the sibling files would stay together if they were moved.

@jjeffryes
Copy link
Collaborator

Git is saying the commits aren't signed, which is blocking the ability to merge. @rmisio can you check to make sure you're set up right for this repo?

@rmisio
Copy link
Contributor Author

rmisio commented Feb 20, 2019

Git is saying the commits aren't signed, which is blocking the ability to merge. @rmisio can you check to make sure you're set up right for this repo?

@hoffmabc I have enough access to push to the repo, but apparently not enough for my PR to merged. Can you check what the issue is?

@hoffmabc
Copy link
Member

@rmisio you do need to sign your commits. i'm not sure if you've been doing it before but that needs to be resolved even if you use admin rights to force push the code. did you change up your git settings?

@rmisio rmisio merged commit 0cb44d0 into master Feb 20, 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.

None yet

3 participants