Migrate the Settings component to the composition API#8708
Conversation
absidue
requested review from
ChunkyProgrammer,
MarmadileManteater,
PikachuEXE,
efb4f5ff-1298-471a-8973-3d47447115dc and
kommunarr
github-actions
bot
added
the
PR: waiting for review
PikachuEXE
left a comment
PikachuEXE
left a comment
There was a problem hiding this comment.
check that the layout and the back button work in both desktop and mobile view
This reads like there is a back button on desktop... which there isn't one I think?
@PikachuEXE i think this still counts as desktop? VirtualBoxVM_LkWHomcbSn.mp4Idk if this weird stuff is even related to this PR? Dropdown opens on a weird place VirtualBoxVM_ACKIYNAU3S.mp4Text is black? VirtualBoxVM_IDAQmBWvLb.mp4 |
|
@efb4f5ff-1298-471a-8973-3d47447115dc |
|
He., sorry for the confusion, in hindsight my sentence in the testing section was badly phrased. I had intended to ask you to test both desktop and mobile views and specifically the navigation behaviour e.g. side bar updates while scrolling on desktop, opening the settings in the mobile view shows the menu first, clicking on a link shows that section, clicking back takes you back to the menu, etc, as those are things that would be affected by the changes in the second commit. |
Can you reproduce it on the development branch? |
efb4f5ff-1298-471a-8973-3d47447115dc
left a comment
efb4f5ff-1298-471a-8973-3d47447115dc
left a comment
There was a problem hiding this comment.
Idk if this weird stuff is even related to this PR?
Can you reproduce it on the development branch?
Yes, so this PR is good to go
github-actions
bot
removed
the
PR: waiting for review
5 participants
Pull Request Type
Description
This pull request migrates the Settings component to the composition API. Currently the Settings component does a lot of direct DOM manipulations in the FtSettingsMenu component (
querySelector,getElementById, adding and removing classes and focusing), so I replaced that in a second commit to use template refs and props, to decouple the two components a bit and make future maintenance easier. Another change is that the icons are now passed in fully from the Settings component into the FtSettingsMenu component, instead of just the name strings being passed through and the full array assembled in the FtSettingsMenu component as that will make a future change that I am planning to do of directly importing the icons where they are used and referencing the import/variable in the component and template directly (currently we register them locally and then have FontAwesome look them up by name at runtime).Testing
Open the settings, check that the layout and the back button work in both desktop and mobile view. Additionally check that changing the settings sort setting works correctly.
Desktop