Skip to content
This repository has been archived by the owner on Aug 1, 2022. It is now read-only.

chore(ui): typing project creation #951

Merged
merged 18 commits into from Sep 29, 2020
Merged

chore(ui): typing project creation #951

merged 18 commits into from Sep 29, 2020

Conversation

sarahscott
Copy link
Contributor

@sarahscott sarahscott commented Sep 23, 2020

Opened a can of worms with this one:

  • Dropdown.svelte & Option.svelte
  • Tooltip.svelte
  • ProjectCreation.svelte & smaller children components
  • Fix bug in ProjectCreation wherein localState store persists after cancel - to reproduce on master or here:
    - Click "New Project"
    - Choose an existing project with multiple branches
    - Select a branch that isn't master
    - Click "Cancel" or close the modal another way
    - Click "New Project" & existing project again - the branches from the first project will still be there even though
    nothing is selected
  • Tests pass

@sarahscott sarahscott self-assigned this Sep 23, 2020
@sarahscott sarahscott added this to To do in β1 Release Preparation via automation Sep 23, 2020
Base automatically changed from sos/project-name-dashes to master September 23, 2020 06:59
@sarahscott sarahscott marked this pull request as ready for review September 24, 2020 23:27
@sarahscott sarahscott requested review from rudolfs, MeBrei, juliendonck, NunoAlexandre and xla and removed request for rudolfs and MeBrei September 25, 2020 16:43
@sarahscott sarahscott moved this from To do to In progress in β1 Release Preparation Sep 25, 2020
Copy link
Member

@rudolfs rudolfs left a comment

Choose a reason for hiding this comment

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

Great work!

Noted some very minor things, should be good to merge otherwise.

ui/Modal/ProjectCreation.svelte Outdated Show resolved Hide resolved
ui/src/project.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
ui/src/style.ts Outdated Show resolved Hide resolved
ui/Modal/ProjectCreation.svelte Outdated Show resolved Hide resolved
ui/Modal/ProjectCreation.svelte Outdated Show resolved Hide resolved
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

Good stuff!!

tsconfig.json Outdated Show resolved Hide resolved
ui/DesignSystem/Component/Dropdown.svelte Show resolved Hide resolved
Copy link
Contributor

@FintanH FintanH left a comment

Choose a reason for hiding this comment

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

Just having a peek

ui/DesignSystem/Component/Dropdown.svelte Show resolved Hide resolved
ui/DesignSystem/Component/Dropdown.svelte Show resolved Hide resolved
ui/src/project.ts Show resolved Hide resolved
Copy link
Member

@juliendonck juliendonck left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. Little nitpick, I find the enum name CSSPosition a bit clunky. It's not really a CSSPosition. It's the position of the tooltip.

Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

☢ 🌘 😌 🌃

@sarahscott
Copy link
Contributor Author

@juliendonck the CSSPosition type is just a reference to css's position-setting properties - top, bottom, left, right - for any component to use, not just the Tooltip. Maybe the Tooltip warrants its own type though 🤔

@sarahscott sarahscott merged commit dd0ed21 into master Sep 29, 2020
β1 Release Preparation automation moved this from In progress to Done Sep 29, 2020
@sarahscott sarahscott deleted the sos/more-typing branch September 29, 2020 19:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants