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 react + UI #1

Merged
merged 31 commits into from
May 8, 2020
Merged

initial react + UI #1

merged 31 commits into from
May 8, 2020

Conversation

saihaj
Copy link
Member

@saihaj saihaj commented Apr 27, 2020

pretty basic UI allows to display the overlay, editor in bottom and script for generating css files from local storage setup

pretty basic UI allows to display the overlay, editor in bottom and script for generating css files from local storage setup
Copy link
Member

@Harjot1Singh Harjot1Singh left a comment

Choose a reason for hiding this comment

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

  • Check indentation
  • Use CapitalCase for files that export classes (and components)

src/App.css Outdated Show resolved Hide resolved
src/App.css Show resolved Hide resolved
src/App.css Outdated Show resolved Hide resolved
src/App.css Outdated Show resolved Hide resolved
src/App.css Outdated Show resolved Hide resolved
src/App.css Outdated Show resolved Hide resolved
src/App.css Outdated Show resolved Hide resolved
src/components/generate.js Outdated Show resolved Hide resolved
src/components/generate.js Outdated Show resolved Hide resolved
src/components/preview.css Outdated Show resolved Hide resolved
@bhajneet
Copy link
Member

bhajneet commented Apr 30, 2020

  • Rename "Line Options" tab to "Preview"
  • Add "section headers" to separate options in each Tab (e.g. in Layout tab there could be "overlay, window, and text sections")
  • Add "ratio" option to "Preview" tab, which allows 16:9 (horizontal), 9:16 (vertical), 18:9, 21:9, and 4:3 options in the dropdown
  • Clarify "preview" of overlay by adding a background and some padding. There should be a clear "border" for where the overlay is displayed in it's chosen ratio. This ratio must not change, though can be resized based on what available space there is.
  • Layout > Window Display should show "flex" and "inline-block" as options only.
  • Layout - there is no "overlay height" option. So cannot make a full screen overlay. See variable --overlay-height which can be "auto" or "100vh" so could be a dropdown for "auto" and "100%".
  • Layout > Vertical/Horizontal padding - the vertical padding should not be affecting horizontal. Either have a button that locks horizontal to match vertical (making horizontal padding not user-modifiable and automatically matching vertical padding slider) or split the two. I would recommend adding a "lock" button (on by default) or simply having one padding slider for vertical and horizontal padding. Lock button is preferable.
  • Cannot test many options until there are background color options for overlay, window, and text.
  • Punjabi and Hindi fonts are incorrect. They don't match "Desktop"
  • Need to add "icons" to each option to match Desktop settings style
  • Lots of wasted horizontal space in bottom panel. Either move panel to be vertically on left/right of the preview or keep the bottom panel and wrap options in at least 3 columns or whatever the browser window's width allows
  • Add tab for "share, save, download, export, etc." of overlay theme that was "generated" by this tool

@bhajneet
Copy link
Member

Cannot scroll or resize bottom panel when there is too much content in the preview. The preview needs to hide it's "overflow" once it's gone past it's "aspect-ratio" size. Slider of bottom tab needs to always take precedence.

image

@saihaj
Copy link
Member Author

saihaj commented Apr 30, 2020

  • Layout > Vertical/Horizontal padding - the vertical padding should not be affecting horizontal. Either have a button that locks horizontal to match vertical (making horizontal padding not user-modifiable and automatically matching vertical padding slider) or split the two. I would recommend adding a "lock" button (on by default) or simply having one padding slider for vertical and horizontal padding. Lock button is preferable.

So by default it is locked, if you look at the slider it says "N/A". so once you edit horizontal it just splits the two.... if you want I can just have vertical and horizontal will change accordingly?

@saihaj saihaj requested a review from Harjot1Singh May 1, 2020 21:46
Copy link
Member

@Harjot1Singh Harjot1Singh left a comment

Choose a reason for hiding this comment

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

Really good work! We can get this merged in and get it deployed.

I need to look at the UI, but we can sort that out later.

Code Improvements:

  • I see that you could drastically cut down the code required, if you can figure out how to leverage the DynamicComponent strategy, which is basically a component factory for settings. See the comment in layoutEditor.js for more detail.

  • Namespace CSS classes. So if your file is called LayoutEditor.js and it has a root CSS class called .layout-editor (which I highly recommend doing for most components), then make sure all CSS declarations (except in exceptional circumstances) are namespaced, so that they only affect the page

  • CSS classes should always be lowercase hello-world and not helloWorld. It just tends to be the universal convention.

  • Any files that export a React class or component should be CamelCase, and corresponding CSS files should match case.

  • Make use of whitespace! It's not evil lol, segregate your JSX just like you would when you think about sentences. Optimise your code for reading :)

Other things that we need to get done at some point (let's file some issues):

  • GitHub action builds => build app and store in separate branch (I think we have one other repo that does this, maybe link that)

  • Set up GH pages, choose subdomain and point it here

  • Set up correct linting

  • Maybe even write some unit tests, dum dum dummm, but it's kinda gonna be a drag now 😂

  • Maybe even extend this to build presenter themes too (but let's make sure in doing this, we don't duplicate much code at all)

src/App.css Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/App.js Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/lib/generate.js Outdated Show resolved Hide resolved
src/lib/template.css Outdated Show resolved Hide resolved
src/lib/utils.js Show resolved Hide resolved
src/components/editors/layoutEditor.js Outdated Show resolved Hide resolved
src/components/preview.js Outdated Show resolved Hide resolved
@bhajneet
Copy link
Member

bhajneet commented May 2, 2020

Default Changes

  • Preview > English Translation = True
  • Layout > Justification = Space Evenly
  • Layout > Height = Auto
  • Layout > Width = 100 (currently NaN) - This also breaks the site, I assume implementing ratio will help
  • Layout > Vertical Padding = 10
  • Layout > Horizontal Padding = match vertical
  • Layout, add a "lock" icon button between vertical and horizontal padding which keeps them both in sync, this should be on by default. This should be done for Overlay, Window, and Text

General Changes

  • Don't need any background sizes for overlay/window/text if there is no way to choose a background image
  • Instead of "Layout, Background, Font" perhaps it would be better to re-arrange those options in "Overlay, Window, and Text" tabs
  • Would be nice to see the color selected next to the dropper (perhaps a square icon with the color fill inside it)
  • Secondary font size would be better as a secondary font ratio (45% or 60% of the primary font size) Default can be 60%.
  • Drop color doesn't seem to work
  • Vishraam colors should be a toggle. If toggled off, the variable is probably supposed to be "inherit".
  • Exporting would be even nicer if we could type the name of the theme before creating a file name.
  • Perhaps creating a file name with the windows path in it will automatically save it in the correct location? I.e. %appdata%\Overlay\<themename>.css if it works, would need to detect correct OS of browser.
  • Exported file needs to end each variable line with a semicolon ; otherwise the file won't work
  • Move control panel to left-hand side
  • Re-arrange scrolling tabs at top to vertical tab icons on left of control-panel. Each tab should have an Icon and short text-description underneath.
  • Resizing of control panel is not working, showing "stop/no" symbol. I don't think re-sizing is that important tbh, but if we're not going to allow it, then I don't think we need a special mouse-over icon, can just be a pointer

Thoughts

Can we use custom colors for Overlay/Window/Text swatch ? And if so, can they be different for text pop-up compared to the overlay/window ones?

image

Closing

Very good progress @saihaj ! Great work so far, two thumbs up 👍 👍

This was referenced May 7, 2020
saihaj and others added 5 commits May 7, 2020 20:49
Co-Authored-By: Harjot Singh <contact@harjot.me>
Uses shabadOS-bot and commit message will be the head message. We can change it to something generic if needed or add semver.
@saihaj
Copy link
Member Author

saihaj commented May 8, 2020

I think I took care of most of the things. Linting job is WIP right now. Also, on refreshing the page preview settings are saved but not rendered, need to fix that.
If you find other things please open issue or comment here.

This will be taken care once I fix the bug I mentioned above.

Preview > English Translation = True

This is already matching it by default that is why it is showing NaN

Layout > Horizontal Padding = match vertical

I think this is the default value. This is currently what it has calc( var(--overlay-primary-font-size) * 0.6 )

Secondary font size would be better as a secondary font ratio (45% or 60% of the primary font size) Default can be 60%.

CSS issue I think. @bhajneet you might want to take a look at this once it is merged.

Drop color doesn't seem to work

So we don't want the user to select a color?

Vishraam colors should be a toggle. If toggled off, the variable is probably supposed to be "inherit".

We won't need this if we take care of #4. We can also tackle as a feat in a new issue (if needed).I would prefer #4 over this

Perhaps creating a file name with the windows path in it will automatically save it in the correct location? I.e. %appdata%\Overlay<themename>.css if it works, would need to detect correct OS of browser.

What type of tests should I write?

Maybe even write some unit tests, dum dum dummm, but it's kinda gonna be a drag now

@saihaj saihaj requested a review from Harjot1Singh May 8, 2020 03:25
Copy link
Member

@Harjot1Singh Harjot1Singh left a comment

Choose a reason for hiding this comment

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

Good work. I've opened up issues for any remaining work

@@ -0,0 +1,53 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We need to update this, probably with one similar to mobile (and also update desktop too). shabados/presenter#487

run: npm run build

- name: Deploy to GH Pages
uses: crazy-max/ghaction-github-pages@v1
Copy link
Member

Choose a reason for hiding this comment

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

nice find!

@Harjot1Singh Harjot1Singh merged commit 8789fd2 into shabados:master May 8, 2020
@Harjot1Singh
Copy link
Member

@saihaj @bhajneet please move any unresolved comments into issues.

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