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.

Sign up for GitHub

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

Jump to bottom

Breaking: Convert to JSX (fixes #146) #194

Merged
merged 34 commits into from
Jun 7, 2024
Merged

Breaking: Convert to JSX (fixes #146) #194

merged 34 commits into from
Jun 7, 2024

Conversation

kirsty-hames
Copy link
Contributor

@kirsty-hames kirsty-hames commented May 21, 2024 •
edited
Loading

Fixes #146

Update

  • Converts .hbs template to .jsx
  • View template filenames updated to .jsx
  • BoxMenuModel created
  • BoxMenuView background layers have been moved into the boxMenu template.

@kirsty-hames
JSX conversion
68729e0 
- replace .hbs templates with .jsx
- change view template filenames
- create Boxmenu model
- condense view background style functions that are dependant on background image
@kirsty-hames kirsty-hames self-assigned this May 21, 2024
oliverfoster
oliverfoster requested changes May 22, 2024
View reviewed changes
templates/boxMenuItem.jsx Outdated Show resolved Hide resolved
templates/boxMenuItem.jsx Outdated Show resolved Hide resolved
js/BoxMenuView.js Outdated Show resolved Hide resolved
kirsty-hames and others added 11 commits May 22, 2024 13:13
oliverfoster
oliverfoster approved these changes May 24, 2024
View reviewed changes
Copy link
Member

@oliverfoster oliverfoster left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👀 This is a breaking change as anyone with template overrides will find their menu not working.

guywillis
guywillis reviewed May 24, 2024
View reviewed changes
Copy link
Contributor

@guywillis guywillis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Lovely upgrade, thank you @kirsty-hames.

Some real nit picky comments from the review mostly. Biggest thing is the _boxmenu to _boxMenu change in the js and jsx files.

A separate thought:

  • Should the menu image have styles attached to it to be consistent with elements around it i.e. margin-bottom - perhaps that's more of a theme related comment but thought I'd raise it

js/BoxMenuView.js Outdated Show resolved Hide resolved
js/BoxMenuView.js Outdated Show resolved Hide resolved
templates/boxMenu.jsx Outdated Show resolved Hide resolved
templates/boxMenu.jsx Outdated Show resolved Hide resolved
templates/boxMenu.jsx Outdated Show resolved Hide resolved
templates/boxMenuItem.jsx Outdated Show resolved Hide resolved
templates/boxMenuItem.jsx Outdated Show resolved Hide resolved
templates/boxMenuItem.jsx Outdated Show resolved Hide resolved
templates/boxMenuItem.jsx Outdated Show resolved Hide resolved
templates/boxMenuItem.jsx Outdated Show resolved Hide resolved
@kirsty-hames kirsty-hames changed the title Update: Convert to JSX (fixes #146) Breaking: Convert to JSX (fixes #146) May 28, 2024
kirsty-hames and others added 11 commits May 28, 2024 10:27
@kirsty-hames @guywillis
capitalise Menu to be consistent with schema
0a926d9 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
capitalise Menu to be consistent with schema
001e2d3 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
capitalise Menu to be consistent with schema
a79f97f 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
capitalise Menu to be consistent with schema
bbc82ce 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
capitalise Menu to be consistent with schema
5b10f60 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split image attributes across multiple lines for readability
9cbca4a 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
c58215c 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
5782685 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
565c182 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
7a9c9e9 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
capitalise Menu to be consistent with schema
b6b4db9 
Co-authored-by: Guy Willis <[email protected]>
kirsty-hames and others added 10 commits May 28, 2024 10:33
@kirsty-hames @guywillis
capitalise Menu to be consistent with schema
6154e48 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
add boxmenu class to be consistent with other classes
bb03a51 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
93a7d71 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
4c17f41 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
7761fbb 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
0ae11c5 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
66cf65b 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames @guywillis
split attributes across multiple lines for readability
6c7a51d 
Co-authored-by: Guy Willis <[email protected]>
@kirsty-hames
indent fix
c3dff08
@kirsty-hames
indent fix
b7595b5
@kirsty-hames
Copy link
Contributor Author

Thanks for the thorough review @guywillis. I've implemented your feedback and amended the PR label to 'Breaking' as per @oliverfoster's comment.

@kirsty-hames kirsty-hames requested a review from guywillis May 28, 2024 15:25
guywillis
guywillis approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@guywillis guywillis left a comment

Choose a reason for hiding this comment

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

👍

cahirodoherty-learningpool
cahirodoherty-learningpool approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@joe-allen-89 joe-allen-89 merged commit 7a1c911 into master Jun 7, 2024
@joe-allen-89 joe-allen-89 deleted the issue/146 branch June 7, 2024 08:53
github-actions bot pushed a commit that referenced this pull request Jun 7, 2024
@semantic-release-bot
Chore(release): 7.0.0 [skip ci]
182b229 
# [7.0.0](v6.7.0...v7.0.0) (2024-06-07)

### Breaking

* Convert to JSX (fixes #146) (#194) ([7a1c911](7a1c911)), closes [#146](#146) [#194](#194)
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 7, 2024

🎉 This PR is included in version 7.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cahirodoherty-learningpool cahirodoherty-learningpool cahirodoherty-learningpool approved these changes

@guywillis guywillis guywillis approved these changes

@oliverfoster oliverfoster oliverfoster approved these changes

Assignees

@kirsty-hames kirsty-hames

Labels
breaking enhancement released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSX Conversion
5 participants
@kirsty-hames @oliverfoster @guywillis @cahirodoherty-learningpool @joe-allen-89