Skip to content

Create basic app structure. Add deploy-dev pipeline #7

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

AmsterGet
Copy link
Member

@AmsterGet AmsterGet commented Mar 1, 2023

TODO (will be a part of another PR):

  • Add SCSS
  • Add aliases for main dirs
  • Create common page Layout component

@AmsterGet AmsterGet force-pushed the basic-app-structure branch from 2558b52 to 27c88c9 Compare March 1, 2023 16:09
@AmsterGet AmsterGet force-pushed the basic-app-structure branch from 2a7ce59 to 32bc5a0 Compare April 5, 2023 17:12
@AmsterGet AmsterGet requested review from skyRoma and Nikkov17 April 13, 2023 16:44
@AmsterGet AmsterGet force-pushed the basic-app-structure branch from 75a030e to 7db6e85 Compare April 25, 2023 21:17
@AmsterGet AmsterGet requested a review from Nikkov17 April 26, 2023 18:19
@AmsterGet
Copy link
Member Author

AmsterGet commented Apr 26, 2023

@Nikkov17 @skyRoma
Please check out the latest built version - https://aikray.github.io/menu-bot-front/
Don't forget to view on mobile screens :)

image

@AmsterGet AmsterGet linked an issue Apr 26, 2023 that may be closed by this pull request
@@ -1,4 +1,18 @@
@import "./core/styles/variables.css";

html {
Copy link
Member

Choose a reason for hiding this comment

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

we can combine html, root, body common styles


const cx = classNames.bind(styles);

interface FooterProps {}
Copy link
Member

Choose a reason for hiding this comment

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

is it needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually no, until we will paste some page specific props to Footer.

Copy link
Member

Choose a reason for hiding this comment

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

So let's remove the dead code for now?

Copy link
Member

Choose a reason for hiding this comment

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

Please, change all file names to snake-case

Copy link
Member

Choose a reason for hiding this comment

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

As we agreed before or let's agree this CapitalCamelCase approach just for component file names. Both cases work for me, but they should be consistent across the whole project.

Copy link
Member

Choose a reason for hiding this comment

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

really minor, but could you please make the step names consistent with the 1st action

image

@@ -15,10 +15,16 @@ jobs:
with:
node-version: 18

- name: Configure git profile
Copy link
Member

Choose a reason for hiding this comment

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

Is it really needed now? Since I don't remember that we used it on the previous project. Maybe the issue is in the token permissions or the token type? I think I used the classic token type
image

transform: rotate(360deg);
}
height: 100%;
width: 100%;
Copy link
Member

Choose a reason for hiding this comment

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

Width is 100% by default, sicne it's a <div>, so please remove this prop

children,
}: IInitialDataProviderProps) => {
// TODO: Use city from Telegram
const extractCity = (): string =>
Copy link
Member

@skyRoma skyRoma May 4, 2023

Choose a reason for hiding this comment

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

it everytime will be null for the first visit, so use :string | null and remove as string below.
Or use localStorage.getItem(LOCAL_STORAGE_SAVED_CITY_KEY) || ''

placeholder="Input search text"
allowClear
onChange={onChange}
style={{ width: '100%' }}
Copy link
Member

Choose a reason for hiding this comment

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

Please, use classes for styles

Copy link
Member

Choose a reason for hiding this comment

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

As I can see it is even 100% by default

entities?: string[];
onChange: (event: ChangeEvent<HTMLInputElement>) => void;
onEntityClick?: (entityName: string) => void;
entityLinkTarget: string;
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be an optional property? I suppose you wanted to make this component reusable even without list, just with input? But in general, I would suggest using composition in such cases (to have 2 separate components, search and list). But as for now we don't need just search (without list) let's make all properties required

Copy link
Member

Choose a reason for hiding this comment

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

And please group them together (first fields then methods). And for the future lets place the optional ones below the required ones

export const RestaurantsSearch = () => {
const { selectedCity } = useContext(StoreContext);
const [restaurants, setRestaurants] = useState<string[]>(
restaurantsByCities[selectedCity] || []
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we won't have a city in the App if we don't have any restaurant for this city so you can remove || []

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I didn't get why do we need this additional provider? why not just use only one StoreProvider? For me it looks unnecessary completed

like this for example

import { LOCAL_STORAGE_SAVED_CITY_KEY } from 'providers/store-provider/constants';
import { ReactElement, useCallback, useState } from 'react';

import {
  IStoreContext,
  StoreContext,
  StoreDispatchContext,
} from './storeContext';

interface IStoreProviderProps {
  children: ReactElement;
}

const extractCity = (): string =>
  localStorage.getItem(LOCAL_STORAGE_SAVED_CITY_KEY) || '';

const saveCityLocally = ({ selectedCity }: Partial<IStoreContext>) => {
  localStorage.setItem(LOCAL_STORAGE_SAVED_CITY_KEY, String(selectedCity));
};

const initialStore = {
  selectedCity: extractCity(),
};

export const StoreProvider = ({ children }: IStoreProviderProps) => {
  const [store, setStore] = useState(initialStore);

  const updateStoreProperties = useCallback(
    (storeProperties: Partial<IStoreContext>) => {
      setStore(prevStore => ({
        ...prevStore,
        ...storeProperties,
      }));
      saveCityLocally(storeProperties);
    },
    []
  );

  return (
    <StoreContext.Provider value={store}>
      <StoreDispatchContext.Provider value={updateStoreProperties}>
        {children}
      </StoreDispatchContext.Provider>
    </StoreContext.Provider>
  );
};

selectedCity: string;
}

export type StoreDispatchType = (storeProperty: Partial<IStoreContext>) => void;
Copy link
Member

Choose a reason for hiding this comment

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

This is not used outside, please remove export.
The same for defaultStore below

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.

basic ui. Main page with list of restaurants.
3 participants