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.

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

Update: add data-order to nav button (#61) #80

Merged
merged 3 commits into from
Mar 13, 2024
Merged

Update: add data-order to nav button (#61) #80

merged 3 commits into from
Mar 13, 2024

Conversation

kirsty-hames
Copy link
Contributor

@kirsty-hames kirsty-hames commented Mar 13, 2024

Fixes #61

Update

Hard code data-order as this won't need to be configurable. As it's a development tool, it makes sense this is last in the nav order (alongside any other development/review tools).

The original issue notes that manually adding the data-order had no effect on the nav order however this worked as expected for me. See screen shot below.

nav_bar_display

@kirsty-hames kirsty-hames self-assigned this Mar 13, 2024
@kirsty-hames kirsty-hames changed the title Fix: add data-order to nav button (#61) Update: add data-order to nav button (#61) Mar 13, 2024
Copy link
Contributor

@swashbuck swashbuck left a comment

Choose a reason for hiding this comment

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

👍

I think we can go ahead and remove the .u-float-right style for the button? Or is that there for backwards compatibility before the nav container nav__inner used flexbox?

@kirsty-hames
Copy link
Contributor Author

I think we can go ahead and remove the .u-float-right style for the button? Or is that there for backwards compatibility before the nav container nav__inner used flexbox?

Good spot @swashbuck, I hadn't noticed. Having looked into this, .u-float-right has been removed from all adaptlearning nav button/elements except languagePicker, but most cgkineo plugins still include this. Seeing as data-order takes precedence over CSS float I'm fine to leave this in for legacy. Then again, not sure if these have been left in for legacy reasons or just forgotten about so equally fine to remove if we want to tidy up?

@swashbuck
Copy link
Contributor

I think we can go ahead and remove the .u-float-right style for the button? Or is that there for backwards compatibility before the nav container nav__inner used flexbox?

Good spot @swashbuck, I hadn't noticed. Having looked into this, .u-float-right has been removed from all adaptlearning nav button/elements except languagePicker, but most cgkineo plugins still include this. Seeing as data-order takes precedence over CSS float I'm fine to leave this in for legacy. Then again, not sure if these have been left in for legacy reasons or just forgotten about so equally fine to remove if we want to tidy up?

@kirsty-hames Thanks for doing that research. Maybe this can be a separate cleanup issue where we update all CGKineo buttons at once? If most adaptlearning nav buttons have had .u-float-right removed, then it's most likely an oversight on the CGKineo buttons.

@kirsty-hames
Copy link
Contributor Author

kirsty-hames commented Mar 13, 2024

I think we can go ahead and remove the .u-float-right style for the button? Or is that there for backwards compatibility before the nav container nav__inner used flexbox?

Good spot @swashbuck, I hadn't noticed. Having looked into this, .u-float-right has been removed from all adaptlearning nav button/elements except languagePicker, but most cgkineo plugins still include this. Seeing as data-order takes precedence over CSS float I'm fine to leave this in for legacy. Then again, not sure if these have been left in for legacy reasons or just forgotten about so equally fine to remove if we want to tidy up?

@kirsty-hames Thanks for doing that research. Maybe this can be a separate cleanup issue where we update all CGKineo buttons at once? If most adaptlearning nav buttons have had .u-float-right removed, then it's most likely an oversight on the CGKineo buttons.

Sounds good to me @swashbuck. I'll remove in this instance and raise issues for the other plugins.

@kirsty-hames kirsty-hames merged commit 12d70d6 into master Mar 13, 2024
Copy link
Contributor

@swashbuck swashbuck left a comment

Choose a reason for hiding this comment

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

👍

@kirsty-hames kirsty-hames deleted the issue/61 branch March 13, 2024 16:35
github-actions bot pushed a commit that referenced this pull request Mar 13, 2024
# [3.4.0](v3.3.1...v3.4.0) (2024-03-13)

### Update

* add data-order to nav button (#61) (#80) ([12d70d6](12d70d6)), closes [#61](#61) [#80](#80)
Copy link

🎉 This PR is included in version 3.4.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation button: Add support for data-order
3 participants