-
Notifications
You must be signed in to change notification settings - Fork 6
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
✨ [#1910] Add first NL-DesignSystem components #909
Conversation
afe1dac
to
c7e0daf
Compare
|
d56c54e
to
f495767
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #909 +/- ##
========================================
Coverage 94.66% 94.66%
========================================
Files 830 830
Lines 29236 29236
========================================
Hits 27676 27676
Misses 1560 1560 ☔ View full report in Codecov by Sentry. |
d39d9d7
to
e3d445d
Compare
e3d445d
to
4a90887
Compare
Before reviewing, please read why we are using 'utrecht' here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! You should update the installation instruction for Open Inwoner itself (INSTALL.rst
under "Get the code") to make sure the submodules are also cloned. Otherwise the folder for the design tokens is there but empty. There are two ways:
git clone --recursive [email protected]:maykinmedia/open-inwoner.git
cd open-inwoner
git clone [email protected]:maykinmedia/open-inwoner.git
cd open-inwoner
# initialize submodules
git submodule update --init --recursive
These are equivalent, the difference is a matter of style. I kind of prefer the second, as it's more explicit (it's not like you have to type this again and again). Perhaps include a comment below to explain what those submodules are.
While you're at it, you could remove the $
signs from the code blocks in INSTALL.rst
so that people can actually copy/paste the code snippets.
@@ -46,6 +46,9 @@ WORKDIR /app | |||
COPY ./build /app/build/ | |||
COPY ./*.json ./*.js ./.babelrc /app/ | |||
|
|||
# Clone design token submodule (normally done using git submodule update --init) | |||
RUN git clone https://github.com/maykinmedia/open-inwoner-design-tokens.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how often this repo will be updated, but I think it would be a good idea to pin this to a specific tag. Otherwise every pipeline of Open Inwoner will always pull in the master
branch of the package's repo.
We could probably do something like this: https://stackoverflow.com/a/21699307 (also makes us skip any history, since we're not going to use that in the image anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just created a Tag (v0.0.1) but I wonder: if this will actually still be updated a lot, isn't it better to keep on pulling the Main branch - in these beginning stages? Every commit means a new Tag since they are big changes every time (I think).
In a far(?) future that repo could become the single source of truth (when they will also be used by the designers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already had to update it to version 0.0.2... :-D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could let it point to the main branch if it will be modified a lot, though I am generally wary of not pinning these kinds of dependencies, because I like to know what versions of dependencies I have for a specific image and not have potential unwanted changes be included automatically. If @alextreme approves to use master then it's fine by me.
You could add --depth 1
to git clone
to avoid pulling in the history (https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---depthltdepthgt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already turned it into this "RUN git clone --depth 1 --branch 0.0.2 https://github.com/maykinmedia/open-inwoner-design-tokens.git
" with the tag, but how will we remember the Tag needs to be updated too? Add to both repo's documentations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's indeed quite a chore to keep these kinds of relations in-sync between repositories, this is why for maykin-deployment I also avoid pinning to a specific branch (and why I'd prefer to go for a single combined repo, this would avoid the problem altogether).
For now I approve and prefer pulling from main/master instead of a release-branch. I'll set a reminder to re-evaluate this in half a year, if changes have become incidental then we'll pin this again to a release-branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenbal @alextreme So should I remove the clone-from-Tag thing and revert to what it was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
Looks good overall, just some remarks regarding docker setup/submodules |
8edf96e
to
fa11f0f
Compare
fa11f0f
to
1d2dee6
Compare
issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/1910