Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

DEP Upgrade frontend build stack #229

Merged
merged 1 commit into from
Feb 1, 2023
Merged

DEP Upgrade frontend build stack #229

merged 1 commit into from
Feb 1, 2023

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli marked this pull request as draft January 18, 2023 04:13
@GuySartorelli GuySartorelli force-pushed the pulls/3/frontend-build-stack branch from 4aadde6 to c518ff9 Compare January 19, 2023 01:34
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jan 19, 2023 •
edited
Loading

Keeping this as draft for now - once silverstripe/webpack-config#59 is released this will need yarn upgrade @silverstripe/webpack-config to be run, and its new yarn.lock to be committed.

@GuySartorelli GuySartorelli mentioned this pull request Jan 20, 2023
Merged
GuySartorelli
GuySartorelli commented Jan 26, 2023
View reviewed changes
src/TagField.php
@@ -497,7 +497,7 @@ protected function getOrCreateTag($value)
}

// Create new instance if not yet saved
if ($this->getCanCreate()) {
if ($this->getCanCreate() && $value) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, a new empty value entry was created.

@GuySartorelli GuySartorelli force-pushed the pulls/3/frontend-build-stack branch 2 times, most recently from 3b3859f to 4ffd2c5 Compare January 26, 2023 03:10
@GuySartorelli GuySartorelli marked this pull request as ready for review January 26, 2023 03:10
@GuySartorelli
Copy link
Member Author

Webpack config changes have been tagged and included in this PR

emteknetnz
emteknetnz requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

CSS is wrong, it looks different from CMS 4

This PR:
image

CMS 4:
image

Other issues:

  • When creating a new tag, pressing 'enter' submits the form, when it should instead create the new tag
  • I cannot create new tags after saving a page with tags.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jan 31, 2023 •
edited
Loading

For styling please compare this with the TreeMultiselectField, not with the CMS 4 version. We went through multiple major versions of react-select and it changed a lot of the markdown and styles that came out of the box. I think some leeway for minor changes in styling is acceptable especially given that this is a major upgrade - people will expect things to look a little different. The styling is different but it's not breaking anything by being different.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jan 31, 2023 •
edited
Loading

When creating a new tag, pressing 'enter' submits the form, when it should instead create the new tag

I can't reproduce this. Can you please include steps for reproduction (or just show me it happening since I sit next to you)

I cannot create new tags after saving a page with tags.

Interesting.... I had tested this but maybe I changed something after testing it. For me, it's not that I can't create new tags specifically but I can't select multiple tags. I will resolve this.

@GuySartorelli GuySartorelli force-pushed the pulls/3/frontend-build-stack branch from 4ffd2c5 to c7d7c74 Compare February 1, 2023 00:09
GuySartorelli
GuySartorelli commented Feb 1, 2023
View reviewed changes
client/src/components/TagField.js
Comment on lines -127 to +178
if (!passThroughAttributes.multi && passThroughAttributes.value) {
if (!multi && passThroughAttributes.value) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't a passthrough anymore - missed this the first attempt when I pulled multi out as a distinct variable.

@emteknetnz
Copy link
Member

emteknetnz commented Feb 1, 2023 •
edited
Loading

Can you please include steps for reproduction

Not much beyond "add a tag and press enter" I used firefox if that makes a difference

I'll retest once I look at all the resolved PRs again, we'll assume it's a non-issue for now

@emteknetnz emteknetnz merged commit 249842f into silverstripe:3 Feb 1, 2023
@emteknetnz emteknetnz deleted the pulls/3/frontend-build-stack branch February 1, 2023 21:13
@sabina-talipova sabina-talipova mentioned this pull request May 30, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@emteknetnz emteknetnz emteknetnz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GuySartorelli @emteknetnz